Suppress certain warnings in inactive #if regions without relying on IfConfigDecl
#76327
Conversation
This file contains hidden or 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
Rather than potentially computing ConfiguredRegions multiple times in ASTGen and other queries, cache the result in the ExportedSourceFile. This is temporary; we should bring up a Swift equivalent to the request-evaluator (or bridge to the C++ one) to manage state like this.
Rather than walking into the inactive regions of IfConfigDecls looking for references to a declaration before we diagnose it, go to the syntax tree and look through inactive *and unparsed* regions for identifier tokens that match. If we find one, suppress the diagnostic. This reduces our dependency on IfConfigDecl in the AST, and also makes the same suppression work with code in unparsed regions that had no representation in IfConfigDecl.
In the warning about having no try/throw within the body of a do..catch, replace the walk of the inactive clauses of IfConfigDecl with a syntactic check of inactive and unparsed regions to look for 'try' and 'throw' keywords. This both eliminates a dependency on IfConfigDecl and expands the usefulness of this warning suppression to unparsed code.
DougGregor
requested review from
hborla,
slavapestov,
ahoppen,
bnbarham,
CodaFi,
hamishknight,
rintaro and
xedin
as code owners
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
ahoppen
reviewed
Sep 7, 2024
| /// Search in inactive/unparsed code to look for evidence that something that | ||
| /// looks unused is actually used in some configurations. | ||
| private enum InactiveCodeChecker { | ||
| case name(String) |
There was a problem hiding this comment.
Should this take an Identifier already instead of a String?
Suggested change
| case name(String) | |
| case name(Identifier) |
There was a problem hiding this comment.
Identifier needs either SyntaxText in an arena, or a StaticString. I don't really have either here because I'm coming from a bridged StringRef.
| return false | ||
| } | ||
|
|
||
| for token in syntax.tokens(viewMode: .sourceAccurate) { |
There was a problem hiding this comment.
Using a SyntaxVisitor instead of tokens(viewMode:) is currently faster because it has performance optimizations to reuse memory allocations, in case this is relevant here.
There was a problem hiding this comment.
It's harder to stop the visitor from running when we find something, though, whereas I can easily "break" out of loop that walks over tokens.
DougGregor
deleted the
suppress-warnings-in-inactive-regions-without-ifconfigdecl
branch
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.
For a few categories of warnings, we currently look through inactive code (represented by
IfConfigDecl) to try to determine whether there are other configurations of code that might not have the warning. If we find evidence of that, we suppress the warning. This includes, e.g.,For example, we don't warn about the lack of try/throw in this code:
even if
DEBUGis not set.Reimplement this suppression mechanism in terms of the pure syntax tree with
SwiftIfConfigdeciding which regions are inactive. This eliminates a dependency on having the inactive clauses represented in anIfConfigDeclas well as expanding the suppression to "unparsed" code: