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

[Index] Don't report extensions with nothing to index #34338

Merged
merged 3 commits into from
Oct 19, 2020

Conversation

xymus
Copy link
Contributor

@xymus xymus commented Oct 16, 2020

The indexer was looking into “empty” extensions to public types,
triggering the computation of the extension USR which could fail at
deserializing implementation-only imported types. As a solution,
don’t index extensions with no content to index.

rdar://70225906

@xymus
Copy link
Contributor Author

xymus commented Oct 16, 2020

@swift-ci Please smoke test

@xymus
Copy link
Contributor Author

xymus commented Oct 16, 2020

@rintaro @nathawes I'm not sure how to test this change, would you have suggestions or pointers for me?

@xymus
Copy link
Contributor Author

xymus commented Oct 17, 2020

Nevermind, it looks like there was already a test about this!

@xymus
Copy link
Contributor Author

xymus commented Oct 17, 2020

@swift-ci Please smoke test

@nathawes
Copy link
Contributor

nathawes commented Oct 17, 2020

I think we may still need to still index empty extensions in user source code so global rename invoked on the type being extended will still be able to find and update the type names in the extensions themselves. IndexASTWalker has IsModuleFile and isSystemModule members you can use to refine the check, depending on whether it's an issue deserializing just system modules or user modules too.

The previously failing test was actually using the old indexing API (in sourcekitd) that we don't really use anymore. To add a test for the above change it'd be better to take a look at test/Index/index_swift_only_system_module.swift. You basically want to copy it up to the end of the "Test-1" section and put an empty extension in your equivalent of some-module.swift (which becomes a system module) and one in %s (user code) and change the c-index-test invocation to do -print-record rather than -print-unit. Take a look at test/Index/Store/record-sourcefile.swift for an example of matching against the -print-record output.

Thanks for fixing this, and if you have any questions or want to look through it together feel free to Slack/video chat with me on Monday!

xymus added 3 commits October 17, 2020 15:27
The indexer was looking into “empty” extensions to public types,
triggering computing their USR which could fail at
deserializing the implementation-only imported types. As a solution,
don’t index extensions with nothing to index in system modules.

rdar://70225906
@xymus xymus force-pushed the indexing-empty-extensions branch from 4317034 to f6931c8 Compare October 18, 2020 22:03
@xymus
Copy link
Contributor Author

xymus commented Oct 18, 2020

@swift-ci Please smoke test

@xymus
Copy link
Contributor Author

xymus commented Oct 18, 2020

I tweaked the fix to apply only on serialized module files. We don’t load implementation-only dependencies for local or system serialized modules so this bug could affect any modules, only partial modules should be safe but I wouldn’t expect them to be used here (?).

Thanks for the detailed guide for the test @nathawes! I’ve added record-systemmodule.swift and something to make sure that empty extensions are still recorded for source files. I used a different strategy to get to the serialized module, so let me know if you’d prefer a style closer to the other Index tests and I’ll fix it.

@xymus xymus merged commit 68b790f into swiftlang:main Oct 19, 2020
@xymus xymus deleted the indexing-empty-extensions branch October 19, 2020 14:49
@nathawes
Copy link
Contributor

Looks great - thanks!

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.

2 participants