Canonicalize and type check module params

agu-z commented 4 months ago

We will now canonicalize module param expressions in imports and create defs for them. When a value symbol is looked up, the scope will also return the params def's symbol if it exists, so we can later extend function calls with it (not included in this PR).

New warnings are introduced for when a module expects params but none are provided, or when some are provided but none are expected. Finally, we will fully type check params in imports against the pattern in the module definition.

https://github.com/roc-lang/roc/assets/1724813/95a7fb36-16fb-43a6-9637-180c7a8f9a23

Note: Ability implementations cannot use values from params yet. I decided to postpone this because I'm not sure how to support this, and it seems like a rare case.

github-actions[bot] commented 3 months ago

Thank you for your contribution! Sometimes PRs end up staying open for a long time without activity, which can make the list of open PRs get long and time-consuming to review. To keep things manageable for reviewers, this bot automatically closes PRs that haven’t had activity in 60 days. This PR hasn’t had activity in 30 days, so it will be automatically closed if there is no more activity in the next 30 days. Keep in mind that PRs marked Closed are not deleted, so no matter what, the PR will still be right here in the repo. You can always access it and reopen it anytime you like!

agu-z commented 3 months ago

@ayazhafiz I'm planning to make another PR for module params this week that depends on this. Do you think this is in a mergeable state?

I addressed every comment you made on the initial review, but let me know if you have more.

ayazhafiz commented 3 months ago

@agu-z I would only suggest considering how these will be compiled, since that might change the implementation, but lgtm!

Anton-4 commented 3 months ago

I'll try do a bit of investigating this week on the test failure and we can ignore it if I don't make progress.

Anton-4 commented 3 months ago

Just going to repeat the error here so it comes if we search for it in the future:

 failures:

---- lowlevel_list_calls stdout ----
thread 'lowlevel_list_calls' panicked at crates/valgrind/src/lib.rs:202:9:
`valgrind` exited with exit code 1. valgrind stdout was: ""

valgrind stderr was: "--41196-- VALGRIND INTERNAL ERROR: Valgrind received a signal 11 (SIGSEGV) - exiting
--41196-- si_code=1;  Faulting address: 0x2C9000;  sp: 0x1002db94f0

valgrind: the 'impossible' happened:
   Killed by fatal signal

host stacktrace:
==41196==    at 0x581985FA: ??? (in /usr/libexec/valgrind/memcheck-amd64-linux)
==41196==    by 0x581BA239: ??? (in /usr/libexec/valgrind/memcheck-amd64-linux)
==41196==    by 0x581BAAE3: ??? (in /usr/libexec/valgrind/memcheck-amd64-linux)
==41196==    by 0x58146BE6: ??? (in /usr/libexec/valgrind/memcheck-amd64-linux)
==41196==    by 0x58147D32: ??? (in /usr/libexec/valgrind/memcheck-amd64-linux)
==41196==    by 0x5812B932: ??? (in /usr/libexec/valgrind/memcheck-amd64-linux)
==41196==    by 0x5812C2A0: ??? (in /usr/libexec/valgrind/memcheck-amd64-linux)
==41196==    by 0x5805A431: ??? (in /usr/libexec/valgrind/memcheck-amd64-linux)
==41196==    by 0x5809B269: ??? (in /usr/libexec/valgrind/memcheck-amd64-linux)
==41196==    by 0x580E3F40: ??? (in /usr/libexec/valgrind/memcheck-amd64-linux)

sched status:
  running_tid=1

Thread 1: status = VgTs_Runnable (lwpid 41196)
client stack range: [0x1FFEFF7000 0x1FFF000FFF] client SP: 0x1FFEFF74D0
valgrind stack range: [0x1002CBA000 0x1002DB9FFF] top usage: 18744 of 1048576

Note: see also the FAQ in the source distribution.
It contains workarounds to several common problems.
In particular, if Valgrind aborted or crashed after
identifying problems in your program, there's a good chance
that fixing those problems will prevent Valgrind aborting or
crashing, especially if it happened in m_mallocfree.c.

If that doesn't help, please report this bug to: www.valgrind.org

In the bug report, send all the above text, the valgrind
version, and what OS and version you are using.  Thanks.

"
stack backtrace:
   0: rust_begin_unwind
             at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/std/src/panicking.rs:647:5
   1: core::panicking::panic_fmt
             at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/panicking.rs:72:14
   2: run_with_valgrind
             at ./src/lib.rs:202:9
   3: valgrind_test_linux
             at ./src/lib.rs:120:13
   4: valgrind::valgrind_test
             at ./src/lib.rs:44:9
   5: valgrind::lowlevel_list_calls
             at ./src/lib.rs:496:5
   6: valgrind::lowlevel_list_calls::{{closure}}
             at ./src/lib.rs:495:25
   7: core::ops::function::FnOnce::call_once
             at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/ops/function.rs:250:5
   8: core::ops::function::FnOnce::call_once
             at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

failures:
    lowlevel_list_calls

Anton-4 commented 3 months ago

I switched that workflow over to using nix and it passed :tada:

roc-lang / roc

Canonicalize and type check module params #6861