AST/Sema: Make MemberImportVisibility a migratable feature
#81751
Conversation
tshortli
requested review from
AnthonyLatsis,
hborla,
slavapestov,
xedin and
DougGregor
as code owners
|
@swift-ci please smoke test |
|
@xedin @AnthonyLatsis @DougGregor I'd appreciate a review as I plan to cherry-pick this to |
tshortli
force-pushed
the
migratable-member-import-visibility
branch
from
34369c8 to
c7cb8ef
Compare
|
@swift-ci please smoke test |
lib/Sema/TypeCheckNameLookup.cpp
Outdated
| diagnoseMissingImportsForMember(const ValueDecl *decl, | ||
| SmallVectorImpl<ModuleDecl *> &modulesToImport, | ||
| SourceFile *sf, SourceLoc loc) { | ||
| auto &ctx = sf->getASTContext(); | ||
| auto count = modulesToImport.size(); | ||
| ASSERT(count > 0); | ||
|
|
||
| bool isMigrating = | ||
| ctx.LangOpts.getFeatureState(Feature::MemberImportVisibility) | ||
| .isEnabledForMigration(); |
There was a problem hiding this comment.
@AnthonyLatsis is it currently possible to coalesce the fix-it from multiple different diagnostics (possibilities here are - if I understand correctly - the same id but different arguments, locations, different ids), or we'd just end up adding multiple import <<module>> from that?
There was a problem hiding this comment.
I'd expect we add a walker to detect this similarly to our NonisolatedNonsending one and keep track of unique imports we need to introduce.
There was a problem hiding this comment.
I see, so you think I'd need to change this to emit a single warning per import that needs to be added?
There was a problem hiding this comment.
Yes, something like that, let's wait for @AnthonyLatsis to confirm but I have a suspicion that we might not be able to coalesce the fix-its...
There was a problem hiding this comment.
the same id but different arguments, locations, different ids), or we'd just end up adding multiple
import <<module>>from that?
Yes, we always let primary diagnostics with different locations or messages through, together with their notes. The swift-syntax FixItApplier does not filter out equal insertions either. It totally makes sense to diagnose — and thus point out — all sites that require an import, even if they share the same fix-it. I think we need to resolve this on the tooling side instead of emitting a single, detached diagnostic per fix-it just to help out the tooling.
The question is, can we come up with a counterexample to the conjecture that discarding an insertion (location, replacement text) that we’ve already seen is always the right move?
There was a problem hiding this comment.
I was thinking that we could have an error with a fix-it per file that has a note per use site in that file, this way we preserve all of the locations and don’t have to coalesce anything.
There was a problem hiding this comment.
Yeah, I could invert the diagnostics that way for the purposes of migration only. I'll explore implementing it that way, thanks!
There was a problem hiding this comment.
I've implemented the suggested approach where in migration mode one warning is emitted per module that needs to be imported. Even if the migration infrastructure can handle the duplicate diagnostics that were emitted previously, I think this is going to yield the best developer experience.
The was never invoked because inaccessibility due to SPI protection level is always diagnosed before missing imports are diagnosed. The functionality could therefore not be tested and should be removed.
…ItLoc()`. For clarity, it should just take a `SourceFile`.
Don't skip checking if a feature is enabled for migration when the feature also has an associated language version.
The migration to `MemberImportVisibility` can be performed mechanically by adding missing import declarations, so offer automatic migration for the feature. Resolves rdar://151931597.
tshortli
force-pushed
the
migratable-member-import-visibility
branch
from
c7cb8ef to
aca6046
Compare
|
@swift-ci please smoke test |
|
There have been significant updates, so please take a look again if you reviewed this previously. |
| } | ||
| } | ||
|
|
||
| // Emit one warning for each module that needcs to be imported and emit notes |
The migration to
MemberImportVisibilitycan be performed mechanically by adding missing import declarations, so offer automatic migration for the feature.Also fixes a bug in the migratable feature infrastructure where
hasFeature()would returnfalseincorrectly when a feature is enabled for migration.Resolves rdar://151931597.