Implementation-only import checking for types used in decls #23722

jrose-apple · 2019-04-02T00:01:43Z

Builds on John's #23702 to handle checking of types in decls, the same way we do for access control. Contains more duplicated code than I like, but appears to work, and may provide refactoring hints in the future.

Part of rdar://problem/48991061

jrose-apple · 2019-04-02T00:02:56Z

~~TODO in this PR: test non-public stored properties in frozen structs + classes.~~ Done!

~~TODO in general as part of validation: conformances??~~ WIP in #23808

jrose-apple · 2019-04-02T01:23:20Z

@swift-ci Please test compiler performance

jrose-apple · 2019-04-02T17:24:27Z

From the failed build. Looks like we're probably okay, but there was a small slowdown.

Summary for master full

Unexpected test results, excluded stats for Deferred, Tagged, ProcedureKit, Wordy, CoreStore

Regressions found (see below)

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

Debug-batch

debug-batch 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	27,565,449,278,771	27,752,682,384,129	187,233,105,358	0.68%
LLVM.NumLLVMBytesOutput	972,294,632	972,295,094	462	0.0%
time.swift-driver.wall	2641.7s	2650.7s	9.0s	0.34%

debug-batch detailed

Regressed (3)

name	old	new	delta	delta_pct
AST.NumASTBytesAllocated	67,811,074,426	68,614,261,736	803,187,310	1.18% ⛔
Sema.OverriddenDeclsRequest	7,586,710	7,719,656	132,946	1.75% ⛔
Sema.USRGenerationRequest	12,707,012	12,971,812	264,800	2.08% ⛔

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (97)

name	old	new	delta	delta_pct
AST.NumDecls	80,786	80,786	0	0.0%
AST.NumDependencies	205,072	205,062	-10	-0.0%
AST.NumImportedExternalDefinitions	1,092,562	1,092,562	0	0.0%
AST.NumInfixOperators	31,080	31,080	0	0.0%
AST.NumLinkLibraries	0	0	0	0.0%
AST.NumLoadedModules	244,881	244,881	0	0.0%
AST.NumLocalTypeDecls	108	108	0	0.0%
AST.NumObjCMethods	9,724	9,724	0	0.0%
AST.NumPostfixOperators	18	18	0	0.0%
AST.NumPrecedenceGroups	14,514	14,514	0	0.0%
AST.NumPrefixOperators	70	70	0	0.0%
AST.NumReferencedDynamicNames	92	92	0	0.0%
AST.NumReferencedMemberNames	3,511,542	3,511,542	0	0.0%
AST.NumReferencedTopLevelNames	271,022	271,022	0	0.0%
AST.NumSourceBuffers	331,664	331,664	0	0.0%
AST.NumSourceLines	2,432,547	2,432,547	0	0.0%
AST.NumSourceLinesPerSecond	2,060,772	2,068,449	7,677	0.37%
AST.NumTotalClangImportedEntities	4,136,677	4,149,402	12,725	0.31%
AST.NumUsedConformances	226,385	226,385	0	0.0%
Driver.ChildrenMaxRSS	106,906,710,016	107,152,728,064	246,018,048	0.23%
Driver.DriverDepCascadingDynamic	0	0	0	0.0%
Driver.DriverDepCascadingExternal	0	0	0	0.0%
Driver.DriverDepCascadingMember	0	0	0	0.0%
Driver.DriverDepCascadingNominal	0	0	0	0.0%
Driver.DriverDepCascadingTopLevel	0	0	0	0.0%
Driver.DriverDepDynamic	0	0	0	0.0%
Driver.DriverDepExternal	0	0	0	0.0%
Driver.DriverDepMember	0	0	0	0.0%
Driver.DriverDepNominal	0	0	0	0.0%
Driver.DriverDepTopLevel	0	0	0	0.0%
Driver.NumDriverJobsRun	16,205	16,205	0	0.0%
Driver.NumDriverJobsSkipped	0	0	0	0.0%
Driver.NumDriverPipePolls	161,062	161,736	674	0.42%
Driver.NumDriverPipeReads	177,166	177,685	519	0.29%
Driver.NumProcessFailures	0	0	0	0.0%
Frontend.MaxMallocUsage	692,492,518,216	695,899,811,632	3,407,293,416	0.49%
Frontend.NumInstructionsExecuted	27,565,449,278,771	27,752,682,384,129	187,233,105,358	0.68%
Frontend.NumProcessFailures	0	0	0	0.0%
IRModule.NumIRAliases	105,540	105,540	0	0.0%
IRModule.NumIRBasicBlocks	3,849,136	3,849,136	0	0.0%
IRModule.NumIRComdatSymbols	0	0	0	0.0%
IRModule.NumIRFunctions	1,804,555	1,804,555	0	0.0%
IRModule.NumIRGlobals	1,857,056	1,857,056	0	0.0%
IRModule.NumIRIFuncs	0	0	0	0.0%
IRModule.NumIRInsts	47,456,421	47,456,421	0	0.0%
IRModule.NumIRNamedMetaData	78,402	78,402	0	0.0%
IRModule.NumIRValueSymbols	3,302,061	3,302,061	0	0.0%
LLVM.NumLLVMBytesOutput	972,294,632	972,295,094	462	0.0%
Parse.NumFunctionsParsed	142,585	142,585	0	0.0%
Parse.NumIterableDeclContextParsed	1,004,447	1,004,447	0	0.0%
SILModule.NumSILGenDefaultWitnessTables	0	0	0	0.0%
SILModule.NumSILGenFunctions	932,961	932,961	0	0.0%
SILModule.NumSILGenGlobalVariables	37,189	37,189	0	0.0%
SILModule.NumSILGenVtables	10,694	10,694	0	0.0%
SILModule.NumSILGenWitnessTables	39,362	39,362	0	0.0%
SILModule.NumSILOptDefaultWitnessTables	0	0	0	0.0%
SILModule.NumSILOptFunctions	1,326,972	1,326,972	0	0.0%
SILModule.NumSILOptGlobalVariables	37,947	37,947	0	0.0%
SILModule.NumSILOptVtables	17,207	17,207	0	0.0%
SILModule.NumSILOptWitnessTables	86,613	86,613	0	0.0%
Sema.AccessLevelRequest	2,338,362	2,342,152	3,790	0.16%
Sema.CustomAttrNominalRequest	0	0	0	0.0%
Sema.DefaultAndMaxAccessLevelRequest	54,592	54,597	5	0.01%
Sema.DefaultTypeRequest	318,780	318,780	0	0.0%
Sema.EnumRawTypeRequest	17,024	17,024	0	0.0%
Sema.ExtendedNominalRequest	3,743,430	3,751,599	8,169	0.22%
Sema.InheritedDeclsReferencedRequest	4,300,492	4,318,497	18,005	0.42%
Sema.InheritedTypeRequest	528,692	528,999	307	0.06%
Sema.IsDynamicRequest	1,828,610	1,828,610	0	0.0%
Sema.IsObjCRequest	1,565,869	1,567,151	1,282	0.08%
Sema.MangleLocalTypeDeclRequest	216	216	0	0.0%
Sema.NamedLazyMemberLoadFailureCount	18,930	18,974	44	0.23%
Sema.NamedLazyMemberLoadSuccessCount	17,460,151	17,455,771	-4,380	-0.03%
Sema.NominalTypeLookupDirectCount	29,220,636	29,274,598	53,962	0.18%
Sema.NumConformancesDeserialized	6,559,270	6,606,959	47,689	0.73%
Sema.NumConstraintScopes	15,055,397	15,061,743	6,346	0.04%
Sema.NumConstraintsConsideredForEdgeContraction	41,336,518	41,337,829	1,311	0.0%
Sema.NumDeclsDeserialized	48,193,494	48,555,033	361,539	0.75%
Sema.NumDeclsFinalized	1,644,178	1,644,178	0	0.0%
Sema.NumDeclsTypechecked	895,635	895,635	0	0.0%
Sema.NumDeclsValidated	1,945,079	1,945,079	0	0.0%
Sema.NumFunctionsTypechecked	949,206	949,206	0	0.0%
Sema.NumGenericSignatureBuilders	1,148,631	1,152,503	3,872	0.34%
Sema.NumLazyGenericEnvironments	9,727,871	9,775,858	47,987	0.49%
Sema.NumLazyGenericEnvironmentsLoaded	195,541	195,625	84	0.04%
Sema.NumLazyIterableDeclContexts	6,718,413	6,729,733	11,320	0.17%
Sema.NumLeafScopes	9,805,328	9,810,714	5,386	0.05%
Sema.NumTypesDeserialized	16,175,319	16,237,748	62,429	0.39%
Sema.NumTypesValidated	1,316,479	1,316,479	0	0.0%
Sema.NumUnloadedLazyIterableDeclContexts	4,407,042	4,398,594	-8,448	-0.19%
Sema.RequirementRequest	62,183	62,283	100	0.16%
Sema.SelfBoundsFromWhereClauseRequest	6,263,914	6,289,697	25,783	0.41%
Sema.SetterAccessLevelRequest	137,533	137,533	0	0.0%
Sema.SuperclassDeclRequest	66,990	67,107	117	0.17%
Sema.SuperclassTypeRequest	30,796	30,796	0	0.0%
Sema.TypeDeclsFromWhereClauseRequest	29,940	29,945	5	0.02%
Sema.UnderlyingTypeDeclsReferencedRequest	146,137	146,823	686	0.47%

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	25,587,848,415,902	25,582,356,306,260	-5,492,109,642	-0.02%
LLVM.NumLLVMBytesOutput	779,848,654	779,848,162	-492	-0.0%
time.swift-driver.wall	4598.0s	4604.4s	6.4s	0.14%

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	216,559	216,559	0	0.0%
AST.NumLoadedModules	16,492	16,492	0	0.0%
AST.NumTotalClangImportedEntities	736,667	736,667	0	0.0%
AST.NumUsedConformances	227,331	227,331	0	0.0%
IRModule.NumIRBasicBlocks	3,234,122	3,234,122	0	0.0%
IRModule.NumIRFunctions	1,486,895	1,486,895	0	0.0%
IRModule.NumIRGlobals	1,622,639	1,622,639	0	0.0%
IRModule.NumIRInsts	29,381,150	29,381,150	0	0.0%
IRModule.NumIRValueSymbols	2,896,376	2,896,376	0	0.0%
LLVM.NumLLVMBytesOutput	779,848,654	779,848,162	-492	-0.0%
SILModule.NumSILGenFunctions	653,152	653,152	0	0.0%
SILModule.NumSILOptFunctions	889,962	889,962	0	0.0%
Sema.NumConformancesDeserialized	2,233,184	2,233,184	0	0.0%
Sema.NumConstraintScopes	13,401,956	13,401,956	0	0.0%
Sema.NumDeclsDeserialized	6,020,091	6,020,091	0	0.0%
Sema.NumDeclsValidated	1,038,655	1,038,655	0	0.0%
Sema.NumFunctionsTypechecked	428,737	428,737	0	0.0%
Sema.NumGenericSignatureBuilders	190,265	190,265	0	0.0%
Sema.NumLazyGenericEnvironments	1,244,822	1,244,822	0	0.0%
Sema.NumLazyGenericEnvironmentsLoaded	21,173	21,173	0	0.0%
Sema.NumLazyIterableDeclContexts	765,629	765,629	0	0.0%
Sema.NumTypesDeserialized	3,185,724	3,185,724	0	0.0%
Sema.NumTypesValidated	616,421	616,421	0	0.0%

test/Sema/implementation-only-import-in-decls.swift

slavapestov · 2019-04-02T22:25:44Z

It's a bit surprising to see more AST context memory allocated...

jrose-apple · 2019-04-03T00:24:16Z

I should probably try a dummy PR that makes everything slow to make sure the bot is really working. I trust it to catch drastic slowdowns but everything else is kind of fuzzy.

jrose-apple · 2019-04-03T22:53:42Z

include/swift/AST/AccessScopeChecker.h

+/// to the given callback.
+///
+/// If the callback returns \c false, aborts the traversal.
+class TypeDeclFinder : public ASTWalker, public TypeWalker {


I tried to fold the TypeRepr walking and the Type walking into one helper class, but that seems increasingly shortsighted. I'm going to undo this.

jrose-apple · 2019-04-04T00:33:40Z

@swift-ci Please smoke test

jrose-apple · 2019-04-04T00:33:46Z

@swift-ci Please test source compatibility

jrose-apple · 2019-04-04T01:15:41Z

@swift-ci Please smoke test

jrose-apple · 2019-04-04T01:15:44Z

@swift-ci Please test source compatibility

jrose-apple · 2019-04-04T02:29:13Z

@swift-ci Please test source compatibility

jrose-apple · 2019-04-04T16:44:53Z

Source compat failures are unrelated to this PR (but are concerning).

...for planned reuse in implementation-only import logic. No intended functionality change.

I wish there was more we could share with the upcoming implementation-only checker, but I don't see an obvious way to do it. All the call sites want to know what kind of declaration they're visiting in order to customize the diagnostic, or downgrade something to a warning, or something else. No functionality change.

Based on the existing access checker for types used in decls. There's a common skeleton here but we can't seem to get it out, so for now add a third DeclVisitor to this file. On the plus side, checking this alongside access is an easy way to make sure everything gets checked. Part of rdar://problem/48991061

...which I had exactly backwards.

jrose-apple · 2019-04-06T05:20:29Z

@swift-ci Please test

jrose-apple · 2019-04-06T05:20:50Z

@brentdax and @slavapestov reviewed this as part of #23808 already.

swift-ci · 2019-04-06T05:21:04Z

Build failed
Swift Test OS X Platform
Git Sha - e873ef50c22fcf3e99dbfc7f3dbce5a6951af7fc

swift-ci · 2019-04-06T05:22:33Z

Build failed
Swift Test Linux Platform
Git Sha - 33500efb60707d3f847e6770776b2fadf70d5582

Implementation-only import checking for types used in decls (cherry picked from commit be48d64)

compnerd · 2019-04-08T23:12:47Z

This broke the windows build: https://dev.azure.com/compnerd/windows-swift/_build/results?buildId=1077

FAILED: lib/Sema/CMakeFiles/swiftSema.dir/TypeCheckAccess.cpp.obj 
C:\PROGRA~2\MICROS~1\2017\COMMUN~1\VC\Tools\MSVC\1416~1.270\bin\Hostx64\x64\cl.exe  /nologo /TP -DCMARK_STATIC_DEFINE -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib\Sema -IC:\agent\_work\1\s\swift\lib\Sema -Iinclude -IC:\agent\_work\1\s\swift\include -IC:\agent\_work\1\s\llvm\include -IC:\agent\_work\1\a\llvm\include -IC:\agent\_work\1\s\clang\include -IC:\agent\_work\1\a\llvm\tools\clang\include -IC:\agent\_work\1\s\cmark\src -IC:\agent\_work\1\a\cmark\src -Ic:\agent\_work\1\s\swift-corelibs-libdispatch\src\BlocksRuntime -Ic:\agent\_work\1\s\swift-corelibs-libdispatch -I"C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\\include" -I"C:\Program Files (x86)\Windows Kits\10\\Include\10.0.17763.0\ucrt" -I"C:\Program Files (x86)\Windows Kits\10\\Include\10.0.17763.0\shared" -I"C:\Program Files (x86)\Windows Kits\10\\Include\10.0.17763.0\um" /GS- /Oy /Zc:inline /Zc:strictStrings /Oi /Zc:rvalueCast /W4 -wd4141 -wd4146 -wd4180 -wd4244 -wd4258 -wd4267 -wd4291 -wd4345 -wd4351 -wd4355 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4800 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /we4062 /wd4068 /permissive- -DOBJC_OLD_DISPATCH_PROTOTYPES=0 /MD /Zi /O2 /Ob1   -UNDEBUG  /EHs-c- /GR- -O2 -DLLVM_ON_WIN32 -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_USE_WINAPI_FAMILY_DESKTOP_APP -D_DLL -D_ENABLE_ATOMIC_ALIGNMENT_FIX /GR- -D_HAS_STATIC_RTTI=0 -U_DEBUG -UNDEBUG "-IC:\Program\ Files\ (x86)\Microsoft\ Visual\ Studio\2017\Community\VC\Tools\MSVC\14.16.27023\/include" "-IC:\Program\ Files\ (x86)\Windows\ Kits\10\/Include/10.0.17763.0/ucrt" "-IC:\Program\ Files\ (x86)\Windows\ Kits\10\/Include/10.0.17763.0/shared" "-IC:\Program\ Files\ (x86)\Windows\ Kits\10\/Include/10.0.17763.0/um" -I c:\agent\_work\1\a\icu-63.1\include\unicode -I c:\agent\_work\1\a\icu-63.1\include  -std:c++14 /showIncludes /Folib\Sema\CMakeFiles\swiftSema.dir\TypeCheckAccess.cpp.obj /Fdlib\Sema\CMakeFiles\swiftSema.dir\swiftSema.pdb /FS -c C:\agent\_work\1\s\swift\lib\Sema\TypeCheckAccess.cpp
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023\include\type_traits(616): error C2139: '`anonymous-namespace'::ImplementationOnlyImportChecker::DiagnoseGenerically': an undefined class is not allowed as an argument to compiler intrinsic type trait '__is_convertible_to'
C:\agent\_work\1\s\swift\lib\Sema\TypeCheckAccess.cpp(1484): note: see declaration of '`anonymous-namespace'::ImplementationOnlyImportChecker::DiagnoseGenerically'
C:\agent\_work\1\s\swift\lib\Sema\TypeCheckAccess.cpp(1499): note: see reference to class template instantiation 'std::is_convertible<`anonymous-namespace'::ImplementationOnlyImportChecker::DiagnoseGenerically,`anonymous-namespace'::ImplementationOnlyImportChecker::CheckImplementationOnlyCallback>' being compiled

jrose-apple requested review from slavapestov and rjmccall April 2, 2019 00:01

jrose-apple requested a review from beccadax April 2, 2019 00:05

jrose-apple force-pushed the type-disadvantage branch from 33500ef to 72b9218 Compare April 2, 2019 01:22

beccadax approved these changes Apr 2, 2019

View reviewed changes

test/Sema/implementation-only-import-in-decls.swift Show resolved Hide resolved

jrose-apple commented Apr 3, 2019

View reviewed changes

jrose-apple force-pushed the type-disadvantage branch from 72b9218 to e873ef5 Compare April 4, 2019 00:33

jrose-apple mentioned this pull request Apr 5, 2019

Implementation-only import checking for conformances #23808

Merged

jrose-apple added 4 commits April 5, 2019 20:10

Access checking: Separate TypeDecl-finding from access scope checking

6c05389

...for planned reuse in implementation-only import logic. No intended functionality change.

Fix implementation-only checking for members of frozen structs/classes

4972351

...which I had exactly backwards.

jrose-apple force-pushed the type-disadvantage branch from e873ef5 to 4972351 Compare April 6, 2019 05:19

jrose-apple changed the title ~~[WIP] Implementation-only import checking for types used in decls~~ Implementation-only import checking for types used in decls Apr 6, 2019

jrose-apple marked this pull request as ready for review April 6, 2019 05:20

jrose-apple merged commit be48d64 into swiftlang:master Apr 8, 2019

jrose-apple deleted the type-disadvantage branch April 8, 2019 16:18

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

Merge pull request swiftlang#23722 from jrose-apple/type-disadvantage

0596283

Implementation-only import checking for types used in decls (cherry picked from commit be48d64)

compnerd mentioned this pull request Apr 8, 2019

Sema: move is_convertible outside of class decl #23872

Merged

Implementation-only import checking for types used in decls #23722

Implementation-only import checking for types used in decls #23722

Uh oh!

Conversation

jrose-apple commented Apr 2, 2019

Uh oh!

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

Uh oh!

Uh oh!

jrose-apple commented Apr 2, 2019

Uh oh!

jrose-apple commented Apr 2, 2019

Summary for master full

Debug-batch

debug-batch brief

debug-batch detailed

Release

release brief

release detailed

Uh oh!

Uh oh!

slavapestov commented Apr 2, 2019

Uh oh!

jrose-apple commented Apr 3, 2019

Uh oh!

jrose-apple Apr 3, 2019

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Apr 4, 2019

Uh oh!

jrose-apple commented Apr 4, 2019

Uh oh!

jrose-apple commented Apr 4, 2019

Uh oh!

jrose-apple commented Apr 4, 2019

Uh oh!

jrose-apple commented Apr 4, 2019

Uh oh!

jrose-apple commented Apr 4, 2019

Uh oh!

jrose-apple commented Apr 6, 2019

Uh oh!

jrose-apple commented Apr 6, 2019

Uh oh!

swift-ci commented Apr 6, 2019

Uh oh!

swift-ci commented Apr 6, 2019

Uh oh!

compnerd commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jrose-apple commented Apr 2, 2019 •

edited

Loading

compnerd commented Apr 8, 2019 •

edited

Loading