Skip to content

[SourceKit] Ignore .swiftsourceinfo files in SourceKit #28374

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

Conversation

nathawes
Copy link
Contributor

We only show diagnostics that occur in the primary file, so getting the right source locations for symbols coming from loaded modules has no benefit and a slight performance cost.

Also disable it in swift-ide-test by default to match SourceKit's behavior, with an option to enable it.

We only show diagnostics that occur in the primary file, so getting the right
source locations for symbols coming from loaded modules has no benefit and a
slight performance cost.
@nathawes nathawes requested review from rintaro and nkcsgexi November 20, 2019 01:55
@nathawes
Copy link
Contributor Author

@swift-ci please test

Copy link
Member

@rintaro rintaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

"-Xfrontend");
sourcekitd_request_array_set_string(Args, SOURCEKITD_ARRAY_APPEND,
"-ignore-module-source-info");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!
Because of this, sourcekitd-test was not compatible to older versions of SourceKit (because they don't understand -ignore-module-source-info frontend option).

@nkcsgexi
Copy link
Contributor

nkcsgexi commented Nov 20, 2019

hmm. Did we have particular performance concern for using .swiftsourceinfo file other than code completion? The way I think of .swiftsourceinfo is that it's an implementation detail of the compiler. It should be transparent to most clients of the AST, including SourceKitd, unless it's really necessary. Also, not only diagnostics are using Decl::getLoc(), other sourcekitd requests could also benefit from the now valid location, right?

@DougGregor
Copy link
Member

If SourceKit encounters an ambiguous overload, and the overloaded declarations are in a different module, I would expect that the source info files would allow us to refer to the source locations properly in that other module. Did that not work? It could have been one of the nice side benefits of this work.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - b42f442

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - b42f442

@nathawes
Copy link
Contributor Author

nathawes commented Nov 21, 2019

@DougGregor do you mean for live issues? Diagnostics (including notes) with a location outside the current buffer aren't shown at the moment. But yeah, if they were shown alongside their associated error/warning they would help with navigating to them. We could do the same with jump-to-definition too (i.e. use the location we now get for references to decls in another module coming back from a CursorInfo request to jump directly to it without using the indexer). The problem with both of these at the moment is that those locations may be stale compared to using the indexer - the index data is updated whenever you edit a file, while the swiftsourceinfo is only updated when you build, so we actually want to avoid using its location data in favor of the USR + index for the moment until we have some mechanism to trigger it being updated after the corresponding source files are edited.

That said though Xi, Argyrios and I spoke about this in person yesterday and decided to not disable it by default and do so via a flag/setting as it might be helpful for other SourceKit clients, e.g. SourceKit-LSP which I believe doesn't have background indexing yet (so the index data is equally stale), and other open source tools that use SourceKit as a more general mechanism to get semantic info about Swift source rather than supporting an IDE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants