Skip to content
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

[NFC] Correct assumptions about static AbstractStorageDecls #23724

Merged
merged 5 commits into from Apr 3, 2019

Conversation

Projects
None yet
4 participants
@brentdax
Copy link
Collaborator

brentdax commented Apr 2, 2019

This PR contains some of the refactoring needed to support static subscripts:

  • Modifies AbstractStorageDecl and its subclasses to allow the AST to express the idea of a static subscript.
  • Modifies various parts of the compiler to avoid casting from AbstractStorageDecl to VarDecl before checking staticness. These changes are mostly mechanical, but I've nontrivially refactored one place in override checking.
  • Corrects a place in the constraint solver which assumes the self parameter of a subscript is never a metatype.

It does not modify the parser to accept static modifiers on subscripts, nor does it modify the type checker to allow subscripts to be applied to metatypes, so there should not be a way to create or use a static subscript. It also tries not to add much new code which will only be exercised when handling a static subscript.

I'd like to land these changes early to give them a little more time to bake and to avoid repeated merge conflicts, particularly on the serialization version number. This PR by itself should not cause any behavior changes; #23358, which implements static subscripts, includes tests verifying that the relevant features behave correctly with static subscripts.

This is a draft PR for the moment because I don't want to land it until we have a toolchain of #23358.

brentdax added some commits Mar 16, 2019

[NFC] AST-level support for static subscripts
* Moves the IsStatic flag from VarDecl to AbstractStorageDecl.
* Adds a StaticSubscriptKind to SubscriptDecl.
* Updates serialization for these changes.
* Updates SubscriptDecl constructor call sites for these changes.
[NFC] Remove VarDecl guards on staticness checks
Fixes various places where we assume that only a VarDecl can be static so they operate on any AbstractStorageDecl instead. NFC until static subscripts are added.
[NFC] Correct constraint system assumption
We currently assume that, if a subscript is declared within a value type’s decl, it must need `self` to be passed inout. This isn’t true for static subscripts, because even though the DeclContext is a value type, the metatype is actually a reference type. Skip this check for non-instance members.

NFC until static subscripts are added.
[NFC] Override-check decls by property, not kind
Rather than figuring out which kind of decl we’re using and then testing the traits it has, check each trait once on the decls where the AST can express it.

This change is NFC currently, but will support static subscripts.

@brentdax brentdax requested a review from slavapestov Apr 2, 2019

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

brentdax commented Apr 2, 2019

@swift-ci please smoke test

@brentdax brentdax marked this pull request as ready for review Apr 2, 2019

@brentdax brentdax requested a review from xedin Apr 2, 2019

@xedin

xedin approved these changes Apr 2, 2019

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

brentdax commented Apr 2, 2019

@swift-ci please test source compatibility

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

brentdax commented Apr 2, 2019

@swift-ci please smoke test compiler performance

@swift-ci

This comment has been minimized.

Copy link
Contributor

swift-ci commented Apr 2, 2019

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 (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (3)
name old new delta delta_pct
Frontend.NumInstructionsExecuted 108,843,501,306 108,977,284,857 133,783,551 0.12%
LLVM.NumLLVMBytesOutput 6,152,520 6,152,600 80 0.0%
time.swift-driver.wall 14.6s 14.6s 1.9ms 0.01%

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,032 18,032 0 0.0%
IRModule.NumIRFunctions 10,542 10,542 0 0.0%
IRModule.NumIRGlobals 8,013 8,013 0 0.0%
IRModule.NumIRInsts 313,099 313,099 0 0.0%
IRModule.NumIRValueSymbols 17,618 17,618 0 0.0%
LLVM.NumLLVMBytesOutput 6,152,520 6,152,600 80 0.0%
SILModule.NumSILGenFunctions 5,370 5,370 0 0.0%
SILModule.NumSILOptFunctions 7,036 7,036 0 0.0%
Sema.NumConformancesDeserialized 17,297 17,297 0 0.0%
Sema.NumConstraintScopes 40,586 40,586 0 0.0%
Sema.NumDeclsDeserialized 147,047 147,047 0 0.0%
Sema.NumDeclsValidated 11,580 11,580 0 0.0%
Sema.NumFunctionsTypechecked 2,540 2,540 0 0.0%
Sema.NumGenericSignatureBuilders 5,030 5,030 0 0.0%
Sema.NumLazyGenericEnvironments 33,144 33,144 0 0.0%
Sema.NumLazyGenericEnvironmentsLoaded 2,036 2,036 0 0.0%
Sema.NumLazyIterableDeclContexts 26,980 26,980 0 0.0%
Sema.NumTypesDeserialized 66,840 66,840 0 0.0%
Sema.NumTypesValidated 12,154 12,154 0 0.0%

Release

release brief

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (3)
name old new delta delta_pct
Frontend.NumInstructionsExecuted 142,666,510,658 142,742,171,531 75,660,873 0.05%
LLVM.NumLLVMBytesOutput 7,168,648 7,168,632 -16 -0.0%
time.swift-driver.wall 26.3s 26.3s 46.3ms 0.18%

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,146 2,146 0 0.0%
AST.NumUsedConformances 890 890 0 0.0%
IRModule.NumIRBasicBlocks 21,170 21,170 0 0.0%
IRModule.NumIRFunctions 10,034 10,034 0 0.0%
IRModule.NumIRGlobals 8,107 8,107 0 0.0%
IRModule.NumIRInsts 224,230 224,230 0 0.0%
IRModule.NumIRValueSymbols 17,289 17,289 0 0.0%
LLVM.NumLLVMBytesOutput 7,168,648 7,168,632 -16 -0.0%
SILModule.NumSILGenFunctions 4,176 4,176 0 0.0%
SILModule.NumSILOptFunctions 5,848 5,848 0 0.0%
Sema.NumConformancesDeserialized 12,457 12,457 0 0.0%
Sema.NumConstraintScopes 39,382 39,382 0 0.0%
Sema.NumDeclsDeserialized 32,451 32,451 0 0.0%
Sema.NumDeclsValidated 7,874 7,874 0 0.0%
Sema.NumFunctionsTypechecked 2,204 2,204 0 0.0%
Sema.NumGenericSignatureBuilders 1,718 1,718 0 0.0%
Sema.NumLazyGenericEnvironments 6,871 6,871 0 0.0%
Sema.NumLazyGenericEnvironmentsLoaded 130 130 0 0.0%
Sema.NumLazyIterableDeclContexts 4,259 4,259 0 0.0%
Sema.NumTypesDeserialized 18,144 18,144 0 0.0%
Sema.NumTypesValidated 6,942 6,942 0 0.0%

@brentdax brentdax merged commit dd95a63 into apple:master Apr 3, 2019

5 checks passed

Swift Compiler Performance on macOS Platform (smoke test) Build finished. No test results found.
Details
Swift Source Compatibility Suite on macOS Platform (Debug)
Details
Swift Source Compatibility Suite on macOS Platform (Release)
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.