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.

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

forall quantifier and mandatory declarations for type variables in new analysis #14668

Merged
merged 4 commits into from
Feb 23, 2024

Conversation

cameel
Copy link
Member

@cameel cameel commented Nov 6, 2023

Depends on #14655. Merged.
Fixes #14570.

This PR introduces the new forall quantifier, which can be used to declare type variables used by a function.

Currently it's usable only on free functions. We may want to consider using it for types and instantiations as well, but those already allow declaring variables and I did not touch their syntax. We may also want to allow it inside blocks to get let-polymorphism, but that's also out of scope of this PR.

Having the quantifier, we can start requiring declarations for all type variables. Thanks to this it is now possible to refer to existing type variables and also use type variables in function return types.

@cameel cameel added the has dependencies The PR depends on other PRs that must be merged first label Nov 6, 2023
@cameel cameel requested a review from ekpyron November 6, 2023 13:55
@cameel cameel self-assigned this Nov 6, 2023
Comment on lines 719 to 720
// TMP: Should I adjust the scope to match visibility? Here or in the Scoper?
//_declaration.annotation().scope = quantifier->scope();
Copy link
Member

Choose a reason for hiding this comment

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

Since this is experimental-only, we can also wait and see if and when this will cause problems.
In general rather nice that this approach doesn't seem to take that much hacking in the name and type resolver and all is localized here!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I was expecting way worse.

Though this could grow if we allow using forall with things other than just functions.

Actually, would you use it for types or instantiations as well? Or is the current way of declaring type variables there better? I initially thought that it would fit there, but after thinking about it, it actually just makes things more verbose. Especially since, unlike with functions, repeating a type variable is not really possible in those cases:

type T(A, B) = (A, B)

instantiation T(A: C1, B: C2): C {}

vs

forall (A, B)
type T(A, B) = (A, B)

forall (A: C1, B: C2)
instantiation T(A, B): C {}

or would something like this even make sense?

forall (A, B)
type T = (A, B)

forall (A: C1, B: C2)
instantiation T: C {}

Copy link
Member Author

@cameel cameel Nov 9, 2023

Choose a reason for hiding this comment

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

Since this is experimental-only, we can also wait and see if and when this will cause problems.

After taking a closer look at how the registration and scope() annotation work, I think this will not cause any problems now, but it may make the interpretation of scope() on functions a bit error prone.

The declaration->scope mapping from annotations seems to be the used by name resolver only to construct the reverse scope->declaration mapping and then it's that reverse mapping that's used for name lookups. So this will work just fine.

The issue with code accessing scope() directly, on the other hand, is that it will have to explicitly account for the quantifier. Right now I allowed the quantifier only on free functions and I have not seen any checks that would be affected, but when we reintroduce contracts, this will break a lot of checks whether a function belongs to a contract. Currently they assume that the contract must be the function's scope.

I made a quick attempt to change quantified function scope in Scoper, but this breaks DeclarationRegistrationHelper's assumptions about tree traversal - it assumes that scopes follow the nesting of ScopeOpeners. This can be adjusted, but I did not want the special-casing to spread to multiple places so I reverted that change in the end.

One final thing to keep in mind is also that the scope is visible in the AST output. So it may also break external tools that make assumptions about how functions are nested.

@cameel
Copy link
Member Author

cameel commented Nov 9, 2023

I think that this PR with dependencies now covers everything mentioned in #14570.

Copy link

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Nov 25, 2023
@cameel cameel removed the stale The issue/PR was marked as stale because it has been open for too long. label Nov 25, 2023
@cameel cameel force-pushed the new-analysis-fixed-type-variables branch from 46e1f21 to 5dd1382 Compare November 27, 2023 13:10
@cameel cameel force-pushed the new-analysis-fixed-type-variables branch 3 times, most recently from 6da0faa to 41ae502 Compare November 28, 2023 12:52
@cameel cameel force-pushed the new-analysis-fixed-type-variables branch from 41ae502 to cc1af4a Compare November 28, 2023 15:12
@cameel cameel force-pushed the new-analysis-fixed-type-variables branch from cc1af4a to 6a0121c Compare December 4, 2023 11:59
@cameel cameel force-pushed the new-analysis-forall branch 2 times, most recently from 0f8a5d5 to 73caadc Compare December 5, 2023 12:37
@cameel cameel force-pushed the new-analysis-fixed-type-variables branch from af6e0bb to 0f10c27 Compare December 19, 2023 13:08
Copy link

github-actions bot commented Jan 3, 2024

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Jan 3, 2024
@cameel cameel removed the stale The issue/PR was marked as stale because it has been open for too long. label Jan 3, 2024
Copy link

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Jan 18, 2024
@cameel cameel removed the stale The issue/PR was marked as stale because it has been open for too long. label Jan 18, 2024
@cameel cameel force-pushed the new-analysis-fixed-type-variables branch from 0f10c27 to 81508ab Compare January 26, 2024 18:53
@ekpyron ekpyron force-pushed the new-analysis-fixed-type-variables branch 2 times, most recently from 0bd2b36 to 8e210d2 Compare January 30, 2024 12:55
Base automatically changed from new-analysis-fixed-type-variables to develop January 30, 2024 16:32
@cameel cameel removed the has dependencies The PR depends on other PRs that must be merged first label Feb 12, 2024
@cameel cameel marked this pull request as ready for review February 12, 2024 16:27
@cameel
Copy link
Member Author

cameel commented Feb 12, 2024

Some jobs are failing, but this does not seem related to the PR. Looks like #14839 might not have fixed everything.

@nikola-matic
Copy link
Collaborator

Some jobs are failing, but this does not seem related to the PR. Looks like #14839 might not have fixed everything.

Well, this is lame; looks like the image used in this PR has a different version of Python

circleci@packer-651dab20-1061-caa7-7f59-31ffa2e7d7e2  ~
$ which python
/c/Python311/python

It's 311 instead of 312, so obviously can't find the binary to symlink since it doesn't exist.

@cameel
Copy link
Member Author

cameel commented Feb 12, 2024

Hmmm... so I guess CircleCI has multiple different versions of the Windows image and they assign them randomly to CI runs? This would also explain why miniconda just suddenly disappeared without us touching the config.

If that's the case, we might be best off just doing #14847 right away to get rid of the hard-coded path.

@nikola-matic
Copy link
Collaborator

Hmmm... so I guess CircleCI has multiple different versions of the Windows image and they assign them randomly to CI runs? This would also explain why miniconda just suddenly disappeared without us touching the config.

If that's the case, we might be best off just doing #14847 right away to get rid of the hard-coded path.

Agreed.

@cameel cameel merged commit dba754e into develop Feb 23, 2024
69 checks passed
@cameel cameel deleted the new-analysis-forall branch February 23, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distinguish fixed free types from generic type variables in experimental solidity.
3 participants