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

Type compatibility error message, when reading from .cfg #2757

Merged
merged 7 commits into from
Oct 17, 2023
Merged

Conversation

Kukovec
Copy link
Collaborator

@Kukovec Kukovec commented Oct 12, 2023

  • Tests added for any new code
  • Ran make fmt-fix (or had formatting run automatically on all files edited)
  • Documentation added for any new functionality
  • Entries added to ./unreleased/ for any new functionality

This PR adds an additional type validation check to any CONSTANT overwritten from a .cfg file. Since the typechecking pass precedes the configuration pass, it was otherwise possible to introduce type-inconsistencies, by overwriting a constant declared with type A with a value of type B, which would only fail at the cell-introduction level, with cryptic error messages.

72aaea0 adds support for literals of uninterpreted types within .cfg files (+ tests and some cleanup to use the typed builder smoothly)

closes #2750

@Kukovec Kukovec requested review from thpani and konnov October 12, 2023 12:23
@Kukovec Kukovec requested a review from shonfeder as a code owner October 12, 2023 12:23
@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2023

Codecov Report

Merging #2757 (64671f3) into main (c877256) will decrease coverage by 0.02%.
The diff coverage is 52.77%.

❗ Current head 64671f3 differs from pull request most recent head fb952ff. Consider uploading reports for the commit fb952ff to get more accurate results

@@            Coverage Diff             @@
##             main    #2757      +/-   ##
==========================================
- Coverage   78.85%   78.83%   -0.02%     
==========================================
  Files         464      464              
  Lines       15878    15882       +4     
  Branches     2545     2589      +44     
==========================================
+ Hits        12520    12521       +1     
- Misses       3358     3361       +3     
Files Coverage Δ
...at/forsyte/apalache/tla/pp/TlcConfigImporter.scala 80.48% <88.23%> (-1.34%) ⬇️
...apalache/tla/passes/pp/ConfigurationPassImpl.scala 34.88% <33.33%> (-1.83%) ⬇️
.../forsyte/apalache/infra/tlc/config/TlcConfig.scala 65.85% <10.00%> (-3.38%) ⬇️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@thpani
Copy link
Collaborator

thpani commented Oct 12, 2023

closes #2750

afaiu from #2750 (comment), we still don't parse uninterpreted type literals from TLC configs, so I'm not sure this actually "closes" the issue. We should check with @konnov what the expected outcome of that issue is, or split it into subtasks, where one is preventing the exception (addressed here), and the other is support for the provided spec.

Also, please add a PR description.

@Kukovec
Copy link
Collaborator Author

Kukovec commented Oct 12, 2023

I don't think we'll ever support ULs in .cfg, mainly because .cfg is not Apalache native. As for supporting the spec, given the cfg as it is provided, the cleanest solution is to replace PROC with an alias to Str, since that's the only way you can use TLC's model values.

Copy link
Collaborator

@konnov konnov left a comment

Choose a reason for hiding this comment

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

Looks good! I just left one comment. Thanks for fixing this!

@@ -64,11 +64,11 @@ object ConfigModelValue {
* the name of a model value
*/
case class ConfigModelValue(name: String) extends ConfigConstExpr {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the difference between ConfigModelValue and ConfigStrValue now? It looks like ConfigStrValue handles model values too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ConfigModelValue is what you get when TLC model values are parsed (e.g. a and b in Producers = {a,b}).
ConfigStrValue is what you get when a "-value is parsed (e.g. "abc" or "1_OF_X")

Copy link
Collaborator

@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.

Looks great!

Amazing that it now works with uninterpreted types

test/tla/Bug2750.cfg Outdated Show resolved Hide resolved
@Kukovec Kukovec enabled auto-merge (squash) October 17, 2023 10:04
@Kukovec Kukovec merged commit 16b040d into main Oct 17, 2023
10 checks passed
@Kukovec Kukovec deleted the jk/2750 branch October 17, 2023 12:01
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.

Unexpected exception
4 participants