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

Fix our dear #693 type checker bug #1166

Merged
merged 13 commits into from
Sep 19, 2023
Merged

Fix our dear #693 type checker bug #1166

merged 13 commits into from
Sep 19, 2023

Conversation

bugarela
Copy link
Collaborator

@bugarela bugarela commented Sep 15, 2023

Hello :octocat:

This fixes #693!

The problem was: even though type variables for parameters are free, we were failing to keep track of this fact after substitutions. So, considering the example:

val b(x: int -> str): int = val c = x.keys() { 1 }

Suppose x has type variable t0, which is free. Once we analyze x.keys(), we'll perform a unification and apply substitutions, resulting in t0 being replaced by t1 -> t2 (since x has to be a map in order for it to be an argument for keys()). However, since t1 and t2 are not free names, when we exit the let definition, the types are quantified, and we loose track that those types are related to the parameter type.

The solution is to make sure that, in a substitution like t0 |-> t1 -> t2, if t0 is a free variable, then t1 and t2 also have to be free.

Also, I found another small bug in the integration with Apalache due to the different representation of row types between quint and apalache. The Apalache representation doesn't allow nested rows (that is, a concrete row in the tail). This also adds a small fix for that.

  • 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

@@ -491,7 +491,8 @@ module state {
val poll = state.polls.get(poll_id)
poll.options.listForall(option =>
val optionKey = option._1
val optionSum = option._2
// FIXME: Type annotation below is a workaround, inferred type is too general
val optionSum: int = option._2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This specific case seems different from the others. I'll look at it in a separate issue (to be created). Every other FIXME of this kind has been removed 😄

@bugarela bugarela marked this pull request as ready for review September 19, 2023 00:37
Copy link
Contributor

@thpani thpani left a comment

Choose a reason for hiding this comment

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

This is amazing, great work!

Changes to the typechecker LGTM.

Could you explain where we need the row type simplifier?
afaik, nested rows are already flattened on the Apalache side:
https://github.com/informalsystems/apalache/blob/315d43ad3afca3ed3abbb5e8f2610e3bd9bc9929/tla-io/src/main/scala/at/forsyte/apalache/io/quint/QuintTypeConverter.scala#L60-L73

I've also pushed a small commit to get rid of a reference to #693 in one of the RFCs.

CHANGELOG.md Outdated Show resolved Hide resolved
quint/src/types/simplification.ts Outdated Show resolved Hide resolved
@bugarela
Copy link
Collaborator Author

Could you explain where we need the row type simplifier? afaik, nested rows are already flattened on the Apalache side: https://github.com/informalsystems/apalache/blob/315d43ad3afca3ed3abbb5e8f2610e3bd9bc9929/tla-io/src/main/scala/at/forsyte/apalache/io/quint/QuintTypeConverter.scala#L60-L73

Interesting, I didn't realize we are already doing this in Apalache.

So... I've added this simplification step because I was hitting an error where Apalache complained about mismatching row types, something like "expected Var or Empty, found Row", which I instantly interpreted as nested rows and jumped to a fix. However, I can't reproduce that anymore 🤔. Perhaps it was caused by some intermediate state of my other fix or something like that.

While trying to decide if this simplification is worth keeping, I noticed we do similar processes both before printing rows and before unifying rows. So I'll extract it from this PR and just add it in a separate cleanup PR accounting for all these scenarios.

@bugarela bugarela requested a review from thpani September 19, 2023 12:03
Copy link
Contributor

@thpani thpani left a comment

Choose a reason for hiding this comment

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

Excellent, LGTM!

@bugarela bugarela enabled auto-merge September 19, 2023 12:41
@bugarela bugarela merged commit 12f5814 into main Sep 19, 2023
15 checks passed
@bugarela bugarela deleted the gabriela/fix-693 branch September 19, 2023 13:02
This was referenced Sep 19, 2023
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.

Nested definition quantification allows arbitrary types
2 participants