Skip to content

Automatically import BOOL when strictness is applied#3036

Merged
rv-jenkins merged 6 commits intodevelopfrom
fix2564-importBOOL
Nov 11, 2022
Merged

Automatically import BOOL when strictness is applied#3036
rv-jenkins merged 6 commits intodevelopfrom
fix2564-importBOOL

Conversation

@radumereuta
Copy link
Copy Markdown
Contributor

Fixes: #2564
This is done for the compiled definition so we don't taint program parsing.

@radumereuta radumereuta changed the base branch from master to develop November 8, 2022 19:31
Copy link
Copy Markdown
Contributor

@dwightguth dwightguth left a comment

Choose a reason for hiding this comment

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

We should generally be very careful about automatically importing things.

Before we merge this, please check whether importing BOOL into the main module fixes this issue; if it does, we should probably just fix the error message.

@ehildenb
Copy link
Copy Markdown
Member

@dwightguth you can't even write a requires clause without BOOL being imported. Do we implicitly import bool in other contexts? What about Map module for configurations?

I think we should likely follow whatever we're doing elsewhere. As long as it's documented well, we should be OK with it. But I guess it should be disabled if the user wants too (eg. --no-implicit-imports or extend --no-prelude).

@ehildenb
Copy link
Copy Markdown
Member

The 95% case is that users want to have BOOL implicitly imported, but we should have an escape hatch (credit to Bruce). I think I agree with this.

@dwightguth
Copy link
Copy Markdown
Contributor

Sounds like the consensus is that this should just work, so I'll rescind my review.

@dwightguth dwightguth dismissed their stale review November 10, 2022 16:17

Disagreeing opinions

@radumereuta
Copy link
Copy Markdown
Contributor Author

A summary:

  1. crashing with key not found: _andBool_ when we add strict inside the syntax module is clearly not acceptable.
  2. manually adding imports private BOOL won't work because it's going to taint the program syntax.
$ kast -e 'true andBool false'
`_andBool_`(#token("true","Bool"),#token("false","Bool"))
  1. ResolveStrict generates the rules in place. So importing BOOL only in the main module won't fix the problem.
  2. we have precedence with implicit imports when parsing the configuration. We import MAP.
  3. right now it's failing in the 95% of cases. Let's have a 5% case and see how that workaround should behave. I'm importing the module by name, so if you define your own domains.md with your own BOOL, then you can override the contents.

One thing I agree with is that documentation should be updated, so I pushed another commit. Please re-review.

@rv-jenkins rv-jenkins merged commit 422f020 into develop Nov 11, 2022
@rv-jenkins rv-jenkins deleted the fix2564-importBOOL branch November 11, 2022 15:45
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.

[Bug] [kompile] - Implicit import of the module BOOL

5 participants