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

Extend transitive availability checking to initial value expressions (better version) #22460

Merged
merged 3 commits into from Feb 11, 2019

Conversation

Projects
None yet
5 participants
@jrose-apple
Copy link
Member

jrose-apple commented Feb 7, 2019

Because initial value expressions aren't actually considered within the VarDecl or PatternBindingDecl they're initializing, the existing logic to search for availability attributes wasn't kicking in, leading to errors when a conditionally-unavailable value was used in an initial value expression for a conditionally-unavailable binding. Fix this by walking the enclosing type or extension to find the appropriate PatternBindingDecl.

SR-9867 / rdar://problem/47852718


@brentdax caught some missing cases in the previous version (#22438); this is better.

Verify that a Decl's DeclContext and a DeclContext's parent are in sync
When a Decl is also a DeclContext, these two concepts are identical,
and we rely on that throughout the compiler.

No functionality change; we appear to already be doing this correctly.
@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Feb 7, 2019

@swift-ci Please test

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Feb 7, 2019

@swift-ci Please test source compatibility

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Feb 7, 2019

@swift-ci Please smoke test compiler performance

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Feb 8, 2019

Build comment file:

Summary for master smoketest

Unexpected test results, excluded stats for ReactiveCocoa

No regressions above thresholds

Debug

debug brief

Regressed (0)
name old new delta delta_pct
Improved (1)
name old new delta delta_pct
time.swift-driver.wall 14.6s 14.4s -191.6ms -1.31%
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
name old new delta delta_pct
Frontend.NumInstructionsExecuted 109,907,933,016 109,474,805,646 -433,127,370 -0.39%
LLVM.NumLLVMBytesOutput 6,262,652 6,262,680 28 0.0%

debug detailed

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (23)
name old new delta delta_pct
AST.NumImportedExternalDefinitions 1,110 1,110 0 0.0%
AST.NumLoadedModules 1,038 1,038 0 0.0%
AST.NumTotalClangImportedEntities 4,462 4,462 0 0.0%
AST.NumUsedConformances 886 886 0 0.0%
IRModule.NumIRBasicBlocks 18,010 18,010 0 0.0%
IRModule.NumIRFunctions 10,550 10,550 0 0.0%
IRModule.NumIRGlobals 8,017 8,017 0 0.0%
IRModule.NumIRInsts 312,622 312,622 0 0.0%
IRModule.NumIRValueSymbols 17,630 17,630 0 0.0%
LLVM.NumLLVMBytesOutput 6,262,652 6,262,680 28 0.0%
SILModule.NumSILGenFunctions 5,382 5,382 0 0.0%
SILModule.NumSILOptFunctions 7,052 7,052 0 0.0%
Sema.NumConformancesDeserialized 17,045 17,045 0 0.0%
Sema.NumConstraintScopes 40,604 40,604 0 0.0%
Sema.NumDeclsDeserialized 145,323 145,323 0 0.0%
Sema.NumDeclsValidated 11,580 11,580 0 0.0%
Sema.NumFunctionsTypechecked 2,540 2,540 0 0.0%
Sema.NumGenericSignatureBuilders 5,026 5,026 0 0.0%
Sema.NumLazyGenericEnvironments 32,605 32,605 0 0.0%
Sema.NumLazyGenericEnvironmentsLoaded 2,036 2,036 0 0.0%
Sema.NumLazyIterableDeclContexts 26,470 26,470 0 0.0%
Sema.NumTypesDeserialized 66,189 66,189 0 0.0%
Sema.NumTypesValidated 12,154 12,154 0 0.0%

Release

release brief

Regressed (0)
name old new delta delta_pct
Improved (1)
name old new delta delta_pct
time.swift-driver.wall 26.5s 26.1s -387.2ms -1.46%
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
name old new delta delta_pct
Frontend.NumInstructionsExecuted 140,798,639,276 140,630,386,376 -168,252,900 -0.12%
LLVM.NumLLVMBytesOutput 7,266,132 7,266,168 36 0.0%

release detailed

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (23)
name old new delta delta_pct
AST.NumImportedExternalDefinitions 402 402 0 0.0%
AST.NumLoadedModules 76 76 0 0.0%
AST.NumTotalClangImportedEntities 2,113 2,113 0 0.0%
AST.NumUsedConformances 890 890 0 0.0%
IRModule.NumIRBasicBlocks 20,999 20,999 0 0.0%
IRModule.NumIRFunctions 10,209 10,209 0 0.0%
IRModule.NumIRGlobals 8,084 8,084 0 0.0%
IRModule.NumIRInsts 220,714 220,714 0 0.0%
IRModule.NumIRValueSymbols 17,437 17,437 0 0.0%
LLVM.NumLLVMBytesOutput 7,266,132 7,266,168 36 0.0%
SILModule.NumSILGenFunctions 4,178 4,178 0 0.0%
SILModule.NumSILOptFunctions 5,840 5,840 0 0.0%
Sema.NumConformancesDeserialized 12,279 12,279 0 0.0%
Sema.NumConstraintScopes 39,374 39,374 0 0.0%
Sema.NumDeclsDeserialized 32,114 32,114 0 0.0%
Sema.NumDeclsValidated 7,874 7,874 0 0.0%
Sema.NumFunctionsTypechecked 2,204 2,204 0 0.0%
Sema.NumGenericSignatureBuilders 1,710 1,710 0 0.0%
Sema.NumLazyGenericEnvironments 6,779 6,779 0 0.0%
Sema.NumLazyGenericEnvironmentsLoaded 130 130 0 0.0%
Sema.NumLazyIterableDeclContexts 4,100 4,100 0 0.0%
Sema.NumTypesDeserialized 18,080 18,080 0 0.0%
Sema.NumTypesValidated 6,942 6,942 0 0.0%

@available(OSX, unavailable)
var osx_inner_init_osx = { let inner_var = osx() } // OK
// FIXME: I'm not sure why this produces two errors instead of just one.

This comment has been minimized.

@slavapestov

slavapestov Feb 8, 2019

Member

I think it's because someone is not skipping multi-statement closures somewhere, so the body of the closure is visited again. Just a guess though.

var osx_init_osx = osx() // OK
lazy var osx_lazy_osx = osx() // OK
var osx_init_multi1_osx = osx(), osx_init_multi2_osx = osx() // OK
var (osx_init_deconstruct1_osx, osx_init_deconstruct2_osx) = osx_pair() // OK

This comment has been minimized.

@slavapestov

slavapestov Feb 8, 2019

Member

Just to be sure can you also test a few cases involving patterns like var (foo, _) = ...

This comment has been minimized.

@jrose-apple

jrose-apple Feb 8, 2019

Author Member

Oh, good call!

@slavapestov
Copy link
Member

slavapestov left a comment

My comments are optional.

@@ -856,27 +857,41 @@ static Optional<ASTNode> findInnermostAncestor(
static const Decl *findContainingDeclaration(SourceRange ReferenceRange,
const DeclContext *ReferenceDC,
const SourceManager &SM) {
if (const Decl *D = ReferenceDC->getInnermostDeclarationDeclContext())
auto ContainsReferenceRange = [&](const Decl *D) -> bool {
if (ReferenceRange.isInvalid())

This comment has been minimized.

@brentdax

brentdax Feb 8, 2019

Collaborator

Could you remove this check from the find_if loop by moving the early return later in the function up?

This comment has been minimized.

@jrose-apple

jrose-apple Feb 8, 2019

Author Member

That changes behavior slightly, since in the top-level case it means we return nullptr and in the decl case we return the decl.

This comment has been minimized.

@brentdax

brentdax Feb 8, 2019

Collaborator

Oh, you're right. I think you could still test the range's validity once before calling find_if, but that might not be worth the trouble.

This comment has been minimized.

@jrose-apple

jrose-apple Feb 8, 2019

Author Member

I guess I can remove the other check, even though that makes things slightly less efficient. What do you think?

This comment has been minimized.

@brentdax

brentdax Feb 11, 2019

Collaborator

I don't think you should remove the other check. You should either put an if (ReferenceRange.isInvalid()) return D; before the if (auto * IDC = ...) block so it never calls ContainsReferenceRange if the reference range is invalid (and perhaps assert that fact in ContainsReferenceRange), or stick with this implementation.

Show resolved Hide resolved lib/Sema/TypeCheckAvailability.cpp
DC = DC->getParent();
} while (DC);
DC = D->getDeclContext();
} while (true);

This comment has been minimized.

@brentdax

brentdax Feb 8, 2019

Collaborator

Can this become a for(;;) loop? If not, can it at least be a while(true) loop instead of do {} while(true) so its lack of natural termination is obvious from the start?

This comment has been minimized.

@jrose-apple

jrose-apple Feb 8, 2019

Author Member

Oh. Right.

jrose-apple added some commits Feb 7, 2019

Extend transitive availability checking to initial value expressions
Because initial value expressions aren't actually considered /within/
the VarDecl or PatternBindingDecl they're initializing, the existing
logic to search for availability attributes wasn't kicking in, leading
to errors when a conditionally-unavailable value was used in an
initial value expression for a conditionally-unavailable binding. Fix
this by walking the enclosing type or extension to find the appropriate
PatternBindingDecl.

https://bugs.swift.org/browse/SR-9867
Remove unnecessary work from availability fix-its
As near as I can tell, this was always going to start with
DeclarationToSearch and then end up with DeclarationToSearch, since
it's already a declaration (and we already checked for null earlier).
No test changes observed either.

@jrose-apple jrose-apple force-pushed the jrose-apple:i-cannot-contain-my-excitement branch from 41a3374 to 19c1765 Feb 8, 2019

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Feb 8, 2019

@swift-ci Please smoke test

@shahmishal

This comment has been minimized.

Copy link
Member

shahmishal commented Feb 8, 2019

@jrose-apple CI is currently offline.

@shahmishal

This comment has been minimized.

Copy link
Member

shahmishal commented Feb 8, 2019

@swift-ci Please smoke test

3 similar comments
@shahmishal

This comment has been minimized.

Copy link
Member

shahmishal commented Feb 9, 2019

@swift-ci Please smoke test

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Feb 9, 2019

@swift-ci Please smoke test

@shahmishal

This comment has been minimized.

Copy link
Member

shahmishal commented Feb 11, 2019

@swift-ci Please smoke test

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Feb 11, 2019

@swift-ci Please smoke test macOS

@jrose-apple jrose-apple merged commit cc1800d into apple:master Feb 11, 2019

2 checks passed

Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details

@jrose-apple jrose-apple deleted the jrose-apple:i-cannot-contain-my-excitement branch Feb 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment