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

Don't display undefined variable warnings when evaluating the ? operator #5983

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Jun 1, 2024

#2921 added the ? operator to allow filters to have expressions like ?foo & foo = "foo", where ?foo is true if and only if foo is a defined variable (or an expression where all the variables have been successfully resolved).

However, it doesn't seem to have been working, which I'd spotted in the ocaml-system updates in ocaml/opam-repository#25861. Unfortunately, the failure means that the repository can't be used with --strict.

The fundamental problem is that the warning about undefined variables is displayed when the variables are being resolved, which sadly includes the moment they are being tested with the ? operator. However, when a filter is partially evaluated, the resulting filter will only contain undefined variables (and the post variable), which allows the check added in #5141 to be co-opted to display the unresolved variables after the filter has been reduced, rather than during the reducing.

@dra27 dra27 added this to the 2.2.0~beta3 milestone Jun 1, 2024
@rjbou rjbou self-requested a review June 4, 2024 12:32
@kit-ty-kate kit-ty-kate self-requested a review June 4, 2024 12:33
dra27 added 2 commits June 4, 2024 17:55
?var & var = "foo" should not trigger a warning if var is undefined.
The implementation of the OpamSwitchState.get_dependencies_t displays
warnings on identifier resolution (which, with --strict enabled, are
fatal). This was incorrect, as this includes the resolution of variables
underneath the `?` operator (which tests for undefined variables).

The revised test relies on the fact that everything should have been
resolved except for `post` and displays errors after the filter has been
reduced.
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

thanks!

@kit-ty-kate
Copy link
Member

Thanks!

@kit-ty-kate kit-ty-kate merged commit 945a14f into ocaml:master Jun 4, 2024
29 checks passed
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.

3 participants