Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Canonicalize and type check module params #6861

Merged
merged 46 commits into from
Aug 14, 2024
Merged

Canonicalize and type check module params #6861

merged 46 commits into from
Aug 14, 2024

Conversation

agu-z
Copy link
Collaborator

@agu-z agu-z commented Jul 2, 2024 •
edited
Loading

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.

CleanShot.2024-07-02.at.10.36.29.mp4

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.

agu-z added 19 commits July 2, 2024 04:10
@agu-z
Import params pending def
5e2ccdb
@agu-z
Canonicalize import params record
96e2d32
@agu-z
Qualified lookups return params
9d26adb
@agu-z
Unqualified lookups return params
5aebb7b
@agu-z
can::Expr::ParamsVar for lookups with params
1526fc4
@agu-z
Fix LookedupSymbol reference in docs
d14b1f8
@agu-z
Canonicalize module param patterns
c6e42ec
@agu-z
Parse module params loc and rename to pattern
7ac7215
@agu-z
Reuse record destructure can for module params
3a1d3d4
@agu-z
Add module params to module cache
7174630
@agu-z
Add module param identifiers to solve"s scope
dd0e282
@agu-z
Do not create unnecessary scope in solve run_help
dcb2767
@agu-z
Wrap import params expr so we can constrain later
674adf1
@agu-z
Contrain module params pattern
8f69e75
@agu-z
Reuse Var constrain for ParamsVar
50b1a60
@agu-z
Include param"s record references
8604bb2
@agu-z
Do not report import params symbol as unused
c541dd5
@agu-z
Constrain and solve import params
5ec4b04 
No reporting yet
@agu-z
Module params are not extensible
f0fe0a3
@agu-z agu-z force-pushed the typecheck-module-params branch from 1d0cf4a to c3885c8 Compare July 2, 2024 13:51
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 6, 2024

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
Copy link
Collaborator Author

agu-z commented Aug 7, 2024

@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
Copy link
Member

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

lukewilliamboswell
lukewilliamboswell previously approved these changes Aug 13, 2024
View reviewed changes
Copy link
Collaborator

@lukewilliamboswell lukewilliamboswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😃

@Anton-4
Copy link
Collaborator

Anton-4 commented Aug 13, 2024

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 Anton-4 self-assigned this Aug 13, 2024
@Anton-4
Copy link
Collaborator

Anton-4 commented Aug 13, 2024

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
Copy link
Collaborator

Anton-4 commented Aug 13, 2024 •
edited
Loading

I switched that workflow over to using nix and it passed 🎉

@lukewilliamboswell lukewilliamboswell merged commit 0be4e49 into main Aug 14, 2024
18 checks passed
@lukewilliamboswell lukewilliamboswell deleted the typecheck-module-params branch August 14, 2024 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ayazhafiz ayazhafiz ayazhafiz left review comments

@lukewilliamboswell lukewilliamboswell lukewilliamboswell approved these changes

Assignees

@Anton-4 Anton-4

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@agu-z @ayazhafiz @Anton-4 @lukewilliamboswell