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

jrose-apple · 2019-02-07T22:46:25Z

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.

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 · 2019-02-07T22:46:42Z

@swift-ci Please test

jrose-apple · 2019-02-07T22:46:50Z

@swift-ci Please test source compatibility

jrose-apple · 2019-02-07T23:54:39Z

@swift-ci Please smoke test compiler performance

swift-ci · 2019-02-08T02:14:56Z

Build comment file:

Summary for master smoketest

Unexpected test results, excluded stats for ReactiveCocoa

No regressions above thresholds

Debug
- debug brief
- debug detailed
Release
- release brief
- release detailed

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%

slavapestov · 2019-02-08T03:59:03Z

test/attr/attr_availability_transitive_osx.swift

+@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.


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.

slavapestov · 2019-02-08T03:59:45Z

test/attr/attr_availability_transitive_osx.swift

+  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


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

Oh, good call!

slavapestov

My comments are optional.

beccadax · 2019-02-08T18:21:53Z

lib/Sema/TypeCheckAvailability.cpp

@@ -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())


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

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

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.

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

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.

lib/Sema/TypeCheckAvailability.cpp

beccadax · 2019-02-08T18:27:15Z

lib/Sema/TypeCheckAvailability.cpp

-    DC = DC->getParent();
-  } while (DC);
+    DC = D->getDeclContext();
+  } while (true);


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?

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

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 · 2019-02-08T19:39:46Z

@swift-ci Please smoke test

shahmishal · 2019-02-08T20:17:41Z

@jrose-apple CI is currently offline.

shahmishal · 2019-02-08T23:32:27Z

@swift-ci Please smoke test

shahmishal · 2019-02-09T01:15:25Z

@swift-ci Please smoke test

jrose-apple · 2019-02-09T01:35:26Z

@swift-ci Please smoke test

shahmishal · 2019-02-11T09:56:41Z

@swift-ci Please smoke test

jrose-apple · 2019-02-11T17:41:18Z

@swift-ci Please smoke test macOS

Verify that a Decl's DeclContext and a DeclContext's parent are in sync

ca0aeb2

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 requested review from slavapestov, beccadax and devincoughlin February 7, 2019 22:46

slavapestov reviewed Feb 8, 2019

View reviewed changes

slavapestov approved these changes Feb 8, 2019

View reviewed changes

beccadax reviewed Feb 8, 2019

View reviewed changes

jrose-apple added 2 commits February 8, 2019 11:39

Remove unnecessary work from availability fix-its

19c1765

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 force-pushed the i-cannot-contain-my-excitement branch from 41a3374 to 19c1765 Compare February 8, 2019 19:39

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

jrose-apple deleted the i-cannot-contain-my-excitement branch February 11, 2019 19:12

jrose-apple mentioned this pull request Feb 11, 2019

[5.0] Extend transitive availability checking to initial value expressions #22516

Merged

belkadan mentioned this pull request Feb 7, 2019

[SR-9867] [5.0 Regression] Can't expose unavailable properties marked with unavailable #52273

Closed

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

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

Uh oh!

Conversation

jrose-apple commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Feb 7, 2019

Uh oh!

jrose-apple commented Feb 7, 2019

Uh oh!

jrose-apple commented Feb 7, 2019

Uh oh!

swift-ci commented Feb 8, 2019

Build comment file:

Summary for master smoketest

Debug

debug brief

debug detailed

Release

release brief

release detailed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Feb 8, 2019

Uh oh!

shahmishal commented Feb 8, 2019

Uh oh!

shahmishal commented Feb 8, 2019

Uh oh!

shahmishal commented Feb 9, 2019

Uh oh!

jrose-apple commented Feb 9, 2019

Uh oh!

shahmishal commented Feb 11, 2019

Uh oh!

jrose-apple commented Feb 11, 2019

Uh oh!

Uh oh!

jrose-apple commented Feb 7, 2019 •

edited

Loading