Skip to content

[NFC] Correct assumptions about static AbstractStorageDecls #23724

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 3, 2019
Merged

[NFC] Correct assumptions about static AbstractStorageDecls #23724

merged 5 commits into from
Apr 3, 2019

Conversation

beccadax
Copy link
Contributor

@beccadax beccadax commented Apr 2, 2019 •
edited
Loading

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.

beccadax and others added 5 commits April 1, 2019 18:04
@beccadax
[NFC] AST-level support for static subscripts
972eda2 
* 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.
@beccadax
Update ASTDumper for static subscripts
022e2d3
@beccadax
[NFC] Override-check decls by property, not kind
8bc1887 
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.
@beccadax
[NFC] Remove VarDecl guards on staticness checks
5c03918 
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.
@beccadax
[NFC] Correct constraint system assumption
67af97c 
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.
@beccadax beccadax requested a review from slavapestov April 2, 2019 01:20
@beccadax
Copy link
Contributor Author

beccadax commented Apr 2, 2019

@swift-ci please smoke test

@beccadax beccadax marked this pull request as ready for review April 2, 2019 08:41
@beccadax beccadax requested a review from xedin April 2, 2019 08:44
@beccadax
Copy link
Contributor Author

beccadax commented Apr 2, 2019

@swift-ci please test source compatibility

@beccadax
Copy link
Contributor Author

beccadax commented Apr 2, 2019

@swift-ci please smoke test compiler performance

@swift-ci
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%

@beccadax beccadax merged commit dd95a63 into swiftlang:master Apr 3, 2019
This was referenced Apr 3, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xedin xedin xedin approved these changes

@slavapestov slavapestov slavapestov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@beccadax @swift-ci @xedin @slavapestov