Fix some SourceKit assertion hits #28226

nathawes · 2019-11-13T03:16:53Z

SourceKit's purely syntactic requests don't install a type-checker and so were hitting the below assertion in ValueDecl::getInterfaceType() when they inadvertently invoked some logic that required a type-checker:

assert(ctx.getLegacyGlobalTypeChecker()
         && "The type checker must be installed to make semantic queries!");

This was happening in calls to:

FuncDecl::isStatic(), which was calling getInterfaceType() unnecessarily in order to print it in a diagnostic message.
ASTVerifier::verifyParsed() for initializers and deinitializers which both call isInvalid() when the init/deinit isn't contained in a type/class decl. I just removed these checks since they're already being checked in ASTVerifier::verifyChecked() as well.
ValueDecl::getFormalAccess() to provide the access level in the document structure response. We could avoid this by approximating the access level syntactically, and actually used to do this before getFormalAccess was requestified, but the access level info doesn't seem like it's being used anywhere (at least not in Xcode or Swift Playgrounds) so I just removed it. This method was calling isInvalid() too (which I've replaced with what is hopefully an equivalent check), and also hit problems in inactive ifconfig clauses. It asserts that extensions are already bound and they're not in inactive ifconfig clauses (ASTScope doesn't support this at the moment). This method calls isInvalid() too (though doesn't need to) but also triggers a different assertion if extensions aren't bound, and we don't want to do that here for performance reasons (these requests run on every keypress in the editor). I've resurrected the old inferAccessLevel() logic we used to use prior to getFormalAccess().

Resolves rdar://problem/57202584

nathawes · 2019-11-13T03:17:12Z

@swift-ci please test

johnfairh · 2019-11-13T09:39:37Z

The key.accessibility has been present since the very early days and is relied on by widely-used open-source tools including at least SwiftLint and Jazzy -- it's a big problem to delete it.

Ideally keep the existing behaviour, but restoring the older syntactic algorithm would be OK --- though there was some fallout for us when this last changed.

@marcelofabri fyi

lib/AST/AccessRequests.cpp

CodaFi · 2019-11-13T16:23:03Z

@johnfairh The problem is that the result of this key would not have been stable. There wasn’t a previous syntactic algorithm, there was only a quasi-semantic algorithm that used to get silently bounced out of the type checker with a null answer and whatever fallback that path gave you.

johnfairh · 2019-11-13T16:51:32Z

Right; I think the 'previous syntactic' version that Nathan mentioned is the inferAccessLevel() stuff removed by c51f884, which was not always technically correct but close enough in practice.

marcelofabri · 2019-11-13T17:20:55Z

16 SwiftLint rules use this, as well as Jazzy.

Even though this key would not be stable, I don't remember ever seeing any false positives caused by this. cc @jpsim

nathawes · 2019-11-13T17:40:01Z

Ah ok, thanks @johnfairh! I'll see if @CodaFi's suggestion works because that will probably have less change in behavior and go back to the inferAccessLevel logic if I hit problems.

CodaFi · 2019-11-13T23:05:38Z

The type checker fixes in this round of changers LGTM

nathawes · 2019-11-13T23:16:48Z

@swift-ci please test

swift-ci · 2019-11-13T23:18:41Z

Build failed
Swift Test Linux Platform
Git Sha - 8b9506ed288f15d014f610defb2c61be283343ba

swift-ci · 2019-11-14T00:46:07Z

Build failed
Swift Test OS X Platform
Git Sha - 8b9506ed288f15d014f610defb2c61be283343ba

CodaFi

Formalizing this: TypeChecker parts LGTM. Can't speak for the syntax model.

It was being used purely to get the name of the type context for a diagnostic message. SourceKit's syntactic-only requests were hitting an assertion when this diagnostic was triggered because they don't set up a type checker.

SourceKit doesn't install a typechecker for its syntactic requests so was hitting assertions in these methods if any typechecking was triggered.

nathawes · 2019-11-16T02:18:06Z

Ok, so after thinking about this a bit more I don't think we can keep using ValueDecl::getFormalAccess() after all. To use it we need to (and recently started to) bind extensions to avoid hitting the reasonable assertion in ExtensionDecl::getBoundNominal() that extensions have actually been bound when getFormalAccess() calls it. The commit that added that assertion (here) also modfied the Sourcekitd's syntactic requests to bind extensions to avoid tripping it. These requests are intended to be run on every keypress in the editor though, so we shouldn't be doing name lookup (which may require module loading) to keep them as fast as possible.

The newest commit stops binding extensions and restores the equivalent of the old inferAccessLevel() functions we used to use prior to the switch to getFormalAccess() but with a few fixes to match its behavior where they'd diverged. It does as much as it can syntactically, and simply doesn't report the access level in cases where it couldn't be computed without name-lookup.

@slavapestov are you ok with this? Or should I try to parameterize getFormalAccess() to have a 'syntactic-only' mode or something to share the code? (Just checking since you deduped this in the first place).

nathawes · 2019-11-16T02:32:44Z

@swift-ci please test

swift-ci · 2019-11-16T02:33:25Z

Build failed
Swift Test OS X Platform
Git Sha - dd9e11818bed8ccc19dd41c67e2eba94f6676373

swift-ci · 2019-11-16T02:35:07Z

Build failed
Swift Test Linux Platform
Git Sha - dd9e11818bed8ccc19dd41c67e2eba94f6676373

It looks like we recently started binding extensions to their nominals in order to continue to compute access levels via ValueDecl::getFormalAccess() after an assertion was added to enforce that bindExtensions had been called before anything tried to call ExtensionDecl::getBoundNominal() - which getFormalAccess() depends on. Sourcekitd's syntactic requests are made on every keypress in the editor though, so we shouldn't do any name binding (which may require module loading) to keep them as fast as possible. This patch restores the old inferAccessLevel() functions we used prior to the switch to ValueDecl::getFormalAccess() (plus a few fixes) that does as much as it can syntactically, without any name binding, and simply doesn't report the access level in cases where it couldn't be computed without name-binding. This also fixes an assertion hit we were getting trying to bind extensions in inactive ifconfig clauses, which ASTScope doesn't support. Resolves rdar://problem/57202584

nathawes · 2019-11-16T21:45:54Z

@swift-ci please test

swift-ci · 2019-11-16T21:46:25Z

Build failed
Swift Test OS X Platform
Git Sha - e8616dcb8c656aa14b1a377def47ba6e8795020d

swift-ci · 2019-11-16T21:48:17Z

Build failed
Swift Test Linux Platform
Git Sha - e8616dcb8c656aa14b1a377def47ba6e8795020d

johnfairh · 2019-11-17T10:01:20Z

tools/SourceKit/lib/SwiftLang/SwiftEditor.cpp

+  if (D->getKind() == DeclKind::Destructor ||
+      D->getKind() == DeclKind::EnumElement) {
+    if (auto container = dyn_cast<NominalTypeDecl>(D->getDeclContext()))
+      return std::max(container->getFormalAccess(), AccessLevel::Internal);


Should this be calling inferAccessSyntactically() again instead of getFormalAccess()?

Yes! Thanks – I'll add a test case and fix.

It should have been recursing into inferAccessSyntactically() to avoid any chance of triggering name lookup.

nathawes · 2019-11-18T19:27:43Z

@swift-ci please test

swift-ci · 2019-11-18T19:28:17Z

Build failed
Swift Test OS X Platform
Git Sha - 11d20b8

swift-ci · 2019-11-18T19:38:07Z

Build failed
Swift Test Linux Platform
Git Sha - 11d20b8

nathawes · 2019-11-19T17:50:34Z

Going to go ahead and merge this to let SourceKit stress tester CI job progress a little further, but if there are any further comments let me know and I'll address them in a follow-up PR.

slavapestov

I think this is fine in principle, but I'm wondering why do you need the access level at all for 'syntactic' requests?

nathawes · 2019-11-19T20:26:09Z

Ideally we wouldn't. I'd like to remove it, and did in an earlier version of this PR but it's been in the SourceKit response since Swift was open sourced and from what others have said earlier in the comments several open source tools like SwiftLint and Jazzy currently rely on it. Longer term I think it'd be good if we could remove these syntactic requests completely in favor of using SwiftSyntax.

jpsim · 2019-11-19T20:46:38Z

Those open source tools can certainly evolve and use preferred mechanisms to access that information. That migration path isn't completely clear to me, especially as SwiftSyntax grows its capabilities to match everything SourceKit's syntactic requests can provide.

nathawes changed the title ~~Fix some SourceKit assertion hits~~ [WIP] Fix some SourceKit assertion hits Nov 13, 2019

CodaFi reviewed Nov 13, 2019

View reviewed changes

lib/AST/AccessRequests.cpp Outdated Show resolved Hide resolved

nathawes force-pushed the assertion-fixes branch from 8b9506e to dd9e118 Compare November 13, 2019 23:01

nathawes changed the title ~~[WIP] Fix some SourceKit assertion hits~~ Fix some SourceKit assertion hits Nov 14, 2019

nathawes marked this pull request as ready for review November 14, 2019 02:31

CodaFi approved these changes Nov 14, 2019

View reviewed changes

Nathan Hawes added 2 commits November 15, 2019 18:10

[ASTVerifier] Don't perform semantic requests in verifyParsed()

77daaf3

SourceKit doesn't install a typechecker for its syntactic requests so was hitting assertions in these methods if any typechecking was triggered.

nathawes force-pushed the assertion-fixes branch from dd9e118 to e8616dc Compare November 16, 2019 02:11

nathawes requested a review from slavapestov November 16, 2019 02:18

nathawes force-pushed the assertion-fixes branch from e8616dc to 11d20b8 Compare November 16, 2019 21:45

johnfairh reviewed Nov 17, 2019

View reviewed changes

[IDE] Remove call to getFormalAccess() in inferAccessSyntactically()

43523df

It should have been recursing into inferAccessSyntactically() to avoid any chance of triggering name lookup.

nathawes merged commit 7b33254 into swiftlang:master Nov 19, 2019

nathawes deleted the assertion-fixes branch November 19, 2019 17:50

slavapestov reviewed Nov 19, 2019

View reviewed changes

nathawes mentioned this pull request Nov 19, 2019

[DNM][SourceKit] Log attempts to mangle a module instead of llvm_unreachable for SourceKit/LSP #28051

Closed

CodaFi mentioned this pull request Nov 19, 2019

Remove TypeChecker::ClosuresWithUncomputedCaptures #28367

Merged

nathawes mentioned this pull request Nov 22, 2019

Put freestanding init decls in a test into a type. swiftlang/swift-format#94

Merged

erikkerber mentioned this pull request Mar 5, 2020

Needle generate fails with extension on Swift 5.2 / Xcode 11.4 uber/needle#331

Closed

Fix some SourceKit assertion hits #28226

Fix some SourceKit assertion hits #28226

Uh oh!

Conversation

nathawes commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nathawes commented Nov 13, 2019

Uh oh!

johnfairh commented Nov 13, 2019

Uh oh!

Uh oh!

CodaFi commented Nov 13, 2019

Uh oh!

johnfairh commented Nov 13, 2019

Uh oh!

marcelofabri commented Nov 13, 2019

Uh oh!

nathawes commented Nov 13, 2019

Uh oh!

CodaFi commented Nov 13, 2019

Uh oh!

nathawes commented Nov 13, 2019

Uh oh!

swift-ci commented Nov 13, 2019

Uh oh!

swift-ci commented Nov 14, 2019

Uh oh!

CodaFi left a comment

Choose a reason for hiding this comment

Uh oh!

nathawes commented Nov 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nathawes commented Nov 16, 2019

Uh oh!

swift-ci commented Nov 16, 2019

Uh oh!

swift-ci commented Nov 16, 2019

Uh oh!

nathawes commented Nov 16, 2019

Uh oh!

swift-ci commented Nov 16, 2019

Uh oh!

swift-ci commented Nov 16, 2019

Uh oh!

johnfairh Nov 17, 2019

Choose a reason for hiding this comment

Uh oh!

nathawes Nov 18, 2019

Choose a reason for hiding this comment

Uh oh!

nathawes commented Nov 18, 2019

Uh oh!

swift-ci commented Nov 18, 2019

Uh oh!

swift-ci commented Nov 18, 2019

Uh oh!

nathawes commented Nov 19, 2019

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

nathawes commented Nov 19, 2019

Uh oh!

jpsim commented Nov 19, 2019

Uh oh!

Uh oh!

nathawes commented Nov 13, 2019 •

edited

Loading

nathawes commented Nov 16, 2019 •

edited

Loading