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

Check access control for the generic requirements of subscripts and typealiases #23726

Merged
merged 2 commits into from Apr 2, 2019

Conversation

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

jrose-apple commented Apr 2, 2019

Oops.

This is a little more involved than it should have been because the requirements on a generic typealias don't always carry a Type anymore; sometimes all you have is the TypeRepr. That should still be okay in practice as long as we don't start doing that for var/let, which can have part of a type be inferred but not all of it.

We don't have an idiom for "this should be an error in the next -swift-version" yet. Maybe it should be 9999, like we're using in the standard library for Apple OS availability?

jrose-apple added some commits Apr 2, 2019

Check access control for the generic requirements of typealiases.
Also oops. This one was a little more involved because the requirements
on a generic typealias don't always carry a Type anymore; sometimes all
you have is the TypeRepr. That should still be okay in practice as long
as we don't start doing that for var/let, which can have part of a type
be inferred but not all of it.

@jrose-apple jrose-apple requested review from slavapestov and brentdax Apr 2, 2019

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Apr 2, 2019

@swift-ci Please test

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Apr 2, 2019

@swift-ci Please test source compatibility

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Apr 2, 2019

@swift-ci Please smoke test compiler performance

@slavapestov
Copy link
Member

slavapestov left a comment

I think long term, we'll want this kind of thing to kick off requests to get referenced decls from where clauses, etc. Right now there's an ordering dependency between having evaluated certain requests and running the code here because it relies in the decl being bound in the TypeReprs, which is fragile. Ideally we'd remove the bound decl from IdentTypeRepr altogether.

@jrose-apple

This comment has been minimized.

Copy link
Member Author

jrose-apple commented Apr 2, 2019

The where clause logic is already doing that, but the inherited types aren't going through that logic.

@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 99,830,338,602 99,823,975,873 -6,362,729 -0.01%
LLVM.NumLLVMBytesOutput 6,152,964 6,152,916 -48 -0.0%
time.swift-driver.wall 13.0s 12.9s -66.0ms -0.51%

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,032 1,032 0 0.0%
AST.NumLoadedModules 828 828 0 0.0%
AST.NumTotalClangImportedEntities 4,210 4,210 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,964 6,152,916 -48 -0.0%
SILModule.NumSILGenFunctions 5,370 5,370 0 0.0%
SILModule.NumSILOptFunctions 7,041 7,041 0 0.0%
Sema.NumConformancesDeserialized 16,171 16,171 0 0.0%
Sema.NumConstraintScopes 40,312 40,312 0 0.0%
Sema.NumDeclsDeserialized 134,587 134,587 0 0.0%
Sema.NumDeclsValidated 11,208 11,208 0 0.0%
Sema.NumFunctionsTypechecked 2,516 2,516 0 0.0%
Sema.NumGenericSignatureBuilders 4,776 4,776 0 0.0%
Sema.NumLazyGenericEnvironments 30,003 30,003 0 0.0%
Sema.NumLazyGenericEnvironmentsLoaded 2,012 2,012 0 0.0%
Sema.NumLazyIterableDeclContexts 23,862 23,862 0 0.0%
Sema.NumTypesDeserialized 59,934 59,934 0 0.0%
Sema.NumTypesValidated 11,902 11,902 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,639,098,985 142,570,846,346 -68,252,639 -0.05%
LLVM.NumLLVMBytesOutput 7,168,728 7,168,716 -12 -0.0%
time.swift-driver.wall 23.5s 23.5s -27.6ms -0.12%

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,728 7,168,716 -12 -0.0%
SILModule.NumSILGenFunctions 4,176 4,176 0 0.0%
SILModule.NumSILOptFunctions 5,849 5,849 0 0.0%
Sema.NumConformancesDeserialized 12,457 12,457 0 0.0%
Sema.NumConstraintScopes 39,382 39,382 0 0.0%
Sema.NumDeclsDeserialized 32,445 32,445 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%

@jrose-apple jrose-apple merged commit 4dcea1c into apple:master Apr 2, 2019

7 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 No test results found.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform No test results found.
Details
Swift Test OS X Platform (smoke test)
Details

@jrose-apple jrose-apple deleted the jrose-apple:substandard-by-any-name branch Apr 2, 2019

jrose-apple added a commit to jrose-apple/swift that referenced this pull request Apr 2, 2019

Merge pull request apple#23726 from jrose-apple/substandard-by-any-name
Check access control for the generic requirements of subscripts and typealiases

(cherry picked from commit 4dcea1c)
@brentdax
Copy link
Collaborator

brentdax left a comment

LGTM.

downgradeToWarning = DowngradeToWarning::Yes;

auto diagID = diag::generic_param_usable_from_inline;
if (downgradeToWarning == DowngradeToWarning::Yes)

This comment has been minimized.

Copy link
@brentdax

brentdax Apr 2, 2019

Collaborator

I wouldn't do it in this PR even if you hadn't merged it already, but it might make sense to build something in to the DiagnosticEngine that downgrades errors to warnings if the language version is not high enough.

TC.diagnoseLanguageChange(/*after:*/5, owner, diag::generic_param_usable_from_inline ...)

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Apr 2, 2019

Author Member

Yeah, that's come up before. The main barrier in this case is that the text for these is slightly different (one is phrased as "oh, you shouldn't do this" and the other is "this doesn't work"), but maybe that could be abstracted out too.

jrose-apple added a commit that referenced this pull request Apr 2, 2019

Merge pull request #23726 from jrose-apple/substandard-by-any-name (#…
…23736)

Check access control for the generic requirements of subscripts and typealiases

(cherry picked from commit 4dcea1c)
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.