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

Constrain operator parameters by their type annotations #1371

Merged
merged 8 commits into from
Feb 16, 2024

Conversation

shonfeder
Copy link
Contributor

@shonfeder shonfeder commented Feb 16, 2024

Closes #1177

Reviewing

The PR is smaller than it looks! Almost everything here is either generated by the parser generator or minor cleanup. Review by commit could be helpful.

The change

The new behavior is easiest to illustrate with reference to the operator
definition used in the regression test added here:

pure def foo(s: str): int = {
  val x = 1.in(s) // We SHOULD identify an error here, since s is annotated as a str
  val y = s + 1   // and NOT identify an error here, incorrectly expecting s to be a set
  y
}

Prior to this fix, typechecking locates the error at val y, because it is not
accounting for the constraint s : str before it begins solving constraints in
the body of the declaration:

error: [QNT000] Couldn't unify int and set
Trying to unify int and Set[int]
Trying to unify (int, int) => int and (Set[int], int) => _t2

4:         val y = s + 1   // and NOT identify an error here, incorrectly expecting s to be a set
                   ^^^^^

error: typechecking failed

With this fix, we now correctly account for the constraint on s, so we catch
the misuse at val x:

error: [QNT000] Couldn't unify set and str
Trying to unify Set[int] and str
Trying to unify (_t0, Set[_t0]) => bool and (int, str) => _t1

3:         val x = 1.in(s) // We SHOULD identify an error here, since s is annotated as a str
                   ^^^^^^^

error: typechecking failed

The fix is achieved primarily via a small improvement to the IR (which does not
impact the integration with Apalache): we add an optional typeAnnotation field
to QuintLambdaParams. This is a natural fix, since we are already parsing
these out together.

Along the way there was some minor cleanup and preparation for
#923

This change will also make it very easy to support selective annotation of parameters or return values, and to support annotating params of lambdas.

  • Tests added for any new code
  • Documentation added for any new functionality
  • Entries added to the respective CHANGELOG.md for any new functionality
  • Feature table on README.md updated for any listed functionality

Shon Feder added 5 commits February 15, 2024 12:08
Keeping the exact same meaning, but greater clarity on the rules.
Remove unneeded mutable vars and checks.
Comment on lines -88 to -89
// stack of names used as parameters and assumptions
protected identOrHoleStack: string[] = []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need this stack, because these are just string values we can grab directly.

Comment on lines 226 to 230
.map(_ => popOrFail(this.paramStack, 'violated grammar of annotated AnnotatedOperDef'))
.reverse()
const res = this.popType().unwrap(() => 'violated grammar of annotated params return type')
const args = params.map(p => {
assert(isAnnotatedDef(p), 'violated grammar of annotated param type')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These unwraps and asserts are just like the ones in all the exitType* methods: the grammar will not bring us to visit this context without these pre-requisites unless we've messed up something badly. It should have no impact on incremental parsing, unless we have a bigger problem that we want to know about.

@@ -64,12 +64,15 @@ describe('findParameterWithId', () => {
const modules = [buildModuleWithDecls(['pure def x(a: int): int = a'])]

it('finds definition for existing id', () => {
const def = findParameterWithId(modules, 1n)

const def = findParameterWithId(modules, 2n)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The number is shifted because the type annotation in the param is now the first leaf we hit while parsing this example.

@shonfeder shonfeder requested a review from bugarela February 16, 2024 11:56
Copy link
Collaborator

@bugarela bugarela left a comment

Choose a reason for hiding this comment

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

Looks amazing ✨. I'm so happy you found such a simple fix here, really worth the time invested!

@shonfeder
Copy link
Contributor Author

Thanks for the review and the fix!

@shonfeder shonfeder enabled auto-merge February 16, 2024 16:29
@shonfeder shonfeder merged commit d2c2dea into main Feb 16, 2024
14 of 15 checks passed
@shonfeder shonfeder deleted the 1177/via-param-annotations branch February 16, 2024 16:40
shonfeder pushed a commit that referenced this pull request Feb 29, 2024
Followup for #1371

I forgot to add visitor logic for the parameter type annotations
shonfeder pushed a commit that referenced this pull request Feb 29, 2024
Followup for #1371

I forgot to add visitor logic for the parameter type annotations
shonfeder pushed a commit that referenced this pull request Feb 29, 2024
Followup for #1371

I forgot to add visitor logic for the parameter type annotations
shonfeder pushed a commit that referenced this pull request Feb 29, 2024
Followup for #1371

I forgot to add visitor logic for the parameter type annotations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange error location given type annotation
2 participants