Relax warning 41 for package variables guarded by a :installed filter #5927
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@kit-ty-kate rightly points out a timing issue which concretely invalidates one of the test cases. The filter above should repeat the guard of the dependency, i.e. instead of: it should be: this also concretely rules out this line from the test: since I'm wondering if this is in fact more related to #5928 - in order to use the variables, we need both that it's unequivocally guarded by Regardless, the test case of |
|
Additional commit added which tweaks the check slightly. Now:
|
kit-ty-kate
reviewed
May 22, 2024
kit-ty-kate
approved these changes
May 22, 2024
kit-ty-kate
reviewed
May 22, 2024
The warnings emitted for guarded-dependency and no-dependency-guarded are not strictly necessary as they are protected by a filter which checks :installed (:installed at present never triggers warning 41). The warning for no-dependency-unguarded is unequivocally correct.
Warning 41 is never triggered for the use of package:installed. Extend
this so that the warning is not triggered for any uses of package:foo
_underneath_ package:installed, i.e.
"%{package:foo}% {package:installed}
can no longer cause warning 41 on package.
The previous change means that a variable will definitely not be expanded unless the package has been installed. However, there is a timing issue which is not desirable - there is no guarantee that if either no-dependency-guarded or no-dependency-installed-only have been installed that they will be installed before or after the current package. This instability is not desirable, either. The check is therefore enhanced slightly so that foo:installed can only be used if depends or depopts in some way mentions the package (rather than doing a full tautology check on depends for whether foo is installed).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The compiler packages contain the following snippet:
but the use of
flexdll:sharetriggers lint warning 41 because the flexdll dependency is guarded byos. However, this warning is a bit obtuse given the argument is guarded byflexdll:installed(and:installedexplicitly doesn't trigger lint warning 41).This PR alters lint warning 41 so that when it evaluates the list of variables used in commands, it scans the filters for instances of
package:installedand then performs a partial evaluation of the filter with thatpackage:installedvariable explicitly set to false (but nothing else set in the environment). If the filter reduces tofalse, then all instances ofpackage:varguarded by the filter are ignored.The first commit adds a test case showing the present behaviour, then the fix shows two of the packages warned being eliminated.