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

Bugfix for #2747 #2748

Merged
merged 7 commits into from
Oct 10, 2023
Merged

Bugfix for #2747 #2748

merged 7 commits into from
Oct 10, 2023

Conversation

Kukovec
Copy link
Collaborator

@Kukovec Kukovec commented Sep 26, 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

closes #2747

No regression tests, since this error only surfaces if an intermediate expression has no rule applicable to it (as is the case in the example provided in #2747), otherwise untyped ephemeral cell expressions do not break the solver.

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.

Thanks for the fix! One quick question on the extra logic in PrimeRule:

Comment on lines 24 to 28
case OperEx(TlaActionOper.prime, nEx @ NameEx(name)) =>
nEx.typeTag match {
case Typed(tt1: TlaType1) => state.setRex(tla.name(name + "'", tt1))
case _ => throw new InternalCheckerError("Internal expressions should be Typed", nEx)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we actually need this extra check?

Feels like it only replicates the check in TlaType1.fromTypeTag, and if we wanted to be consistent, we'd need to replicate this kind of logic across all the rules?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, that's fair, we could just use that.

@Kukovec Kukovec requested a review from thpani October 10, 2023 10:57
@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2023

Codecov Report

Merging #2748 (9af1495) into main (9ec6f9e) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2748      +/-   ##
==========================================
- Coverage   78.88%   78.86%   -0.02%     
==========================================
  Files         464      464              
  Lines       15876    15876              
  Branches     2550     2557       +7     
==========================================
- Hits        12523    12520       -3     
- Misses       3353     3356       +3     
Files Coverage Δ
...t/forsyte/apalache/tla/bmcmt/rules/PrimeRule.scala 66.66% <100.00%> (ø)
...t/forsyte/apalache/tla/bmcmt/rules/SubstRule.scala 60.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

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

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.

👍

@Kukovec Kukovec merged commit 5316435 into main Oct 10, 2023
10 checks passed
@Kukovec Kukovec deleted the jk/2747 branch October 10, 2023 12:35
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.

BMC pass throws TypingException
3 participants