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

CLI Option for Disabling Support for Global Variables #541

Merged
merged 4 commits into from
Oct 6, 2022

Conversation

ArquintL
Copy link
Member

@ArquintL ArquintL commented Oct 4, 2022

This might be another slightly controversial PR :P
I just had again a case where Gobra wanted me to create "fake" source files for some transitively imported packages.
I'm even on the fence with myself whether I should actually do it or use my option but in any case it might be handy to (temporarily or not) disable the support for global variables.
I reject a program if it's using global variables, init postconditions or import preconditions. Did I forget any other check?

@ArquintL ArquintL requested a review from jcp19 October 4, 2022 15:17
Copy link
Contributor

@jcp19 jcp19 left a comment

Choose a reason for hiding this comment

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

It seems to me that what you really want is an option to allow lazy imports; disabling globals seems to be an unfortunate side effect, not the goal. Thus, I think this option could be framed differently and could be called `--lazyImport" - somehow, putting things this way sounds less controversial to me 😄

I agree with the restrictions that you suggest, but I would also check for usages of init blocks.

Finally, if you depend on this, I am ok with merging it but there is one detail that is easy to miss which is that the checks seem to be performed superficially. Thus, a program that has 3 packages like this could never succeed past initialization, but Gobra (with this flag) would not report it:

main/main.go

package main

import "pkg1"

func main() {
    ...
}

pkg1/f.go

package pkg1

import "pkg2"

...

pkg2/f.go

package pkg2

func init() {
    panic()
}

@ArquintL
Copy link
Member Author

ArquintL commented Oct 6, 2022

All suggestions sound good! Enforcing lazy imports for now basically means disabling global variables but it sounds more future proof as we might need to disable additional language features in the future (or reconsider the CLI option).
I'm happy to add another check that disallows init blocks but I agree that a "broken" dependency, i.e. a dependency with a crashing init block, could not be caught this way

Copy link
Contributor

@jcp19 jcp19 left a comment

Choose a reason for hiding this comment

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

LGTM!

In the future, if you think that disallowing global variables is too restrictive, one could allow them and only skip the proof obligations concerning initialization; additionally we could emit a message saying that initialization code is currently not checked when the user passes the --lazyImport flag, similarly to the message that we emit when a user uses closures.

@ArquintL ArquintL merged commit 01c38e0 into master Oct 6, 2022
@ArquintL ArquintL deleted the option-for-disabling-global-vars branch October 6, 2022 09:14
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.

2 participants