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

[SourceKit] Allow module refs to be indexed #19243

Merged
merged 21 commits into from Sep 28, 2018

Conversation

Projects
None yet
2 participants
@rockbruno
Contributor

rockbruno commented Sep 11, 2018

Pitch

This allows Swift to index explicit reference to modules, such as Swift.String.

Resolves SR-8677.

@rockbruno

This comment has been minimized.

Contributor

rockbruno commented Sep 11, 2018

@akyrtzi Here it is
I got the mangler to generate an USR to modules, but I'm not sure if that's the correct way

@akyrtzi akyrtzi self-requested a review Sep 24, 2018

@akyrtzi

Here are some directions:

  • For the module USR, update the clang checkout and use generateFullUSRForTopLevelModuleName(StringRef ModName, raw_ostream &OS) which comes from clang/Index/USRGeneration.h. This will allow us to use same USR and track module references across both Swift and ObjC.
  • For testing, primarily use swift-ide-test, and follow the examples from test/Index directory. Testing core functionality (e.g. how are symbols indexed) should occur via swift-ide-test, and SourceKit testing should be used mainly for verifying that the information goes through the SourceKit responses as expected. From your PR I think the modifications to existing SourceKit tests are sufficient to verify that the information goes through, you don't need to add additional SourceKit test cases.
  • For testing, I recommend to extend your test case with also:
    • Test that module references in import func type of imports also get handled
    • Test that module references in clang submodules (like import SomeClangModule.SubModule) are handled properly. You may find generateFullUSRForModule(const clang::Module *Mod, raw_ostream &OS) useful for this
@rockbruno

This comment has been minimized.

Contributor

rockbruno commented Sep 26, 2018

I had to add a few more methods to treat the clang modules - unit-pcm-dependency.swift is failing, but I wanted to iterate from here:

@@ -134,10 +134,18 @@ static SymbolKind getVarSymbolKind(const VarDecl *VD) {
return SymbolKind::Variable;
}
SymbolInfo index::getSymbolInfoForModule(ModuleEntity Mod) {
SymbolInfo info{ SymbolKind::Unknown, SymbolSubKind::None,

This comment has been minimized.

@rockbruno

rockbruno Sep 26, 2018

Contributor

Should we give different parameters to clang modules?

This comment has been minimized.

@akyrtzi

akyrtzi Sep 26, 2018

Member

We can use the language kind to differentiate, SymbolLanguage::Swift for Swift modules and generic SymbolLanguage::C for clang modules.

This comment has been minimized.

@rockbruno

rockbruno Sep 26, 2018

Contributor

Done!

@@ -13,6 +13,7 @@
#ifndef SWIFT_AST_USRGENERATION_H
#define SWIFT_AST_USRGENERATION_H
#include "swift/AST/Module.h"

This comment has been minimized.

@akyrtzi

akyrtzi Sep 27, 2018

Member

Generally try to keep headers lean, meaning don't include other headers if you can just forward declare the symbols you need instead.
Here you included swift/AST/Module.h so you can reference ModuleEntity, but you can just forward declare it as class ModuleEntity;.
You'd need to include the header itself in the cases where the full definition of ModuleEntity is needed, for example if you use it inside an inline function.

This comment has been minimized.

@rockbruno

rockbruno Sep 27, 2018

Contributor

Ah forgive me, I didn't know you could do that in C++. Changes applied

@@ -13,6 +13,7 @@
#ifndef SWIFT_INDEX_INDEXSYMBOL_H
#define SWIFT_INDEX_INDEXSYMBOL_H
#include "swift/AST/Module.h"

This comment has been minimized.

@akyrtzi

akyrtzi Sep 27, 2018

Member

Forward declare here as well.

@@ -365,6 +365,13 @@ ModuleDecl::ModuleDecl(Identifier name, ASTContext &ctx)
setAccess(AccessLevel::Public);
}
bool ModuleDecl::isClangModule() const {
if (!getFiles().empty())

This comment has been minimized.

@akyrtzi

akyrtzi Sep 27, 2018

Member

Should this be using ModuleDecl::findUnderlyingClangModule() instead ?

This comment has been minimized.

@rockbruno

rockbruno Sep 27, 2018

Contributor

Was the isClangModule logic at

auto isClangModule = [](const ModuleDecl *M) -> bool {
duplicated? I took it from there, but changing it to use findUnderlyingClangModule works as well.

This comment has been minimized.

@akyrtzi

akyrtzi Sep 27, 2018

Member

Was the isClangModule logic duplicated?

Quite possible, utility code in testing tools generally gets less scrutiny than additions in libAST.

Show resolved Hide resolved lib/AST/Module.cpp
@@ -205,6 +203,17 @@ swift::USRGenerationRequest::evaluate(Evaluator &evaluator, const ValueDecl* D)
llvm::SmallString<128> Buffer;
llvm::raw_svector_ostream OS(Buffer);
if (auto ModuleD = dyn_cast<ModuleDecl>(D)) {
StringRef moduleName = ModuleD->getName().str();

This comment has been minimized.

@akyrtzi

akyrtzi Sep 27, 2018

Member

A bit better to use ModuleD->getNameStr()

Show resolved Hide resolved lib/AST/USRGeneration.cpp
Show resolved Hide resolved lib/Index/Index.cpp
Show resolved Hide resolved test/Index/index_module_refs.swift
@@ -264,7 +264,6 @@ class ModuleDecl : public DeclContext, public TypeDecl {
return { Files.begin(), Files.size() };
}
bool isClangModule() const;

This comment has been minimized.

@akyrtzi

akyrtzi Sep 27, 2018

Member

Sorry for being unclear, I actually liked that you added isClangModule() method, I was mainly suggesting to look into implementing this in terms of calling findUnderlyingModule()

This comment has been minimized.

@rockbruno

rockbruno Sep 27, 2018

Contributor

No worries! I've misread it

bool ModuleDecl::isClangModule() const {
if (findUnderlyingClangModule())
return true;
return false;

This comment has been minimized.

@rockbruno

rockbruno Sep 27, 2018

Contributor

(I mainly did this instead of return findUnderlyingClangModule() because I thought the latter might be confusing in terms of the actual return type)

This comment has been minimized.

@akyrtzi

akyrtzi Sep 27, 2018

Member

In such situations I usually write it as:

return findUnderlyingClangModule() != nullptr;
@@ -1290,6 +1289,7 @@ class ModuleEntity {
bool isBuiltinModule() const;
const ModuleDecl *getAsSwiftModule() const;
const clang::Module *getAsClangModule() const;
void *getOpaqueValue() const;

This comment has been minimized.

@akyrtzi

akyrtzi Sep 27, 2018

Member

I'd recommend to make this an inline method (move the definition inline). The definition is simple and inlining it would be good for performance.

import ClangModuleB
// CHECK: [[@LINE-1]]:8 | module/C | ClangModuleB | c:@M@ClangModuleB | Ref | rel: 0
import ClangModuleC.Sub1
// CHECK: [[@LINE-1]]:8 | module/C | ClangModuleC | c:@M@ClangModuleC | Ref | rel: 0

This comment has been minimized.

@akyrtzi

akyrtzi Sep 27, 2018

Member

Here's a nice improvement you can make for how test checking works in this file:

  • Match the USR and add it as a FileCheck variable. So instead of having c:@M@ClangModuleC make it [[ClangModuleC_USR:c:@M@ClangModuleC]]
  • Later in the file, instead of repeating the same USR to match (c:@M@ClangModuleC) you match with the variable you defined earlier ([[ClangModuleC_USR]]).
  • Do the same for the other USRs that get repeated, like for SwiftModuleC.

The benefit of this is that it reduces the duplication of the USR string. Later on, if for some reason we decide to change how the USRs for modules are formed, there will be much fewer places in the test cases that would be necessary to correct.

This comment has been minimized.

@rockbruno

rockbruno Sep 28, 2018

Contributor

Amazing feature. Thanks for the tip!

func myMethod() {
_ = SwiftModuleC.MyGeneric<SwiftModuleC.MyType, MyType>()
// CHECK: [[@LINE-1]]:9 | module/Swift | SwiftModuleC | c:@M@SwiftModuleC | Ref | rel: 0
// CHECK: [[@LINE-2]]:22 | struct/Swift | MyGeneric | s:12SwiftModuleC9MyGenericV | Ref,RelCont | rel: 1

This comment has been minimized.

@akyrtzi

akyrtzi Sep 27, 2018

Member

I think it will be a bit better to keep the test case focused on testing that module references are indexed. Meaning we don't need to check for MyGeneric and MyType here, other test cases should be responsible to make sure that we index these properly.
Essentially I'd recommend to remove the check lines for those.

This comment has been minimized.

@rockbruno

rockbruno Sep 28, 2018

Contributor

Since some of the references rely on getParentDecl(), I was concerned that a change to this feature could potentially break the reference that comes right after it - perhaps I should make a separate test file for this interaction?

This comment has been minimized.

@akyrtzi

akyrtzi Sep 28, 2018

Member

That's fair, it'd be fine to have the additional checks but at least reduce how strict the CHECK string needs to be, for example if we only want to check that the struct reference shows up we could have it like this:

// CHECK: [[@LINE-2]]:22 | struct/Swift | MyGeneric | {{.*}} | Ref

So essentially we care that MyGeneric reference shows up but we don't care to match its USR.

@@ -1288,6 +1289,12 @@ class ModuleEntity {
bool isSystemModule() const;
bool isBuiltinModule() const;
const ModuleDecl *getAsSwiftModule() const;
const clang::Module *getAsClangModule() const;
inline void *getOpaqueValue() const {

This comment has been minimized.

@akyrtzi

akyrtzi Sep 28, 2018

Member

inline is redundant, it's the fact that you have the definition present that makes it inlinable.

// CHECK: [[@LINE-1]]:11 | module/Swift | SwiftModuleC | [[SwiftModuleC_USR]] | Ref | rel: 0
func myMethod() {
_ = SwiftModuleC.MyGeneric<SwiftModuleC.MyType, MyType>()
// CHECK: [[@LINE-1]]:9 | module/Swift | SwiftModuleC | [[SwiftModuleC_USR]] | Ref | rel: 0

This comment has been minimized.

@akyrtzi

akyrtzi Sep 28, 2018

Member

One I think I noticed is that while the other type references have | Ref,RelCont | rel: 1 (a 'RelCont' relation with the containing method), the module references do not have that.
I wouldn't say it is super important to have the containing relation, but could you take a look to see if it is simple to add, so that the module references do not diverge ?
If you see other test cases, we check for relations by using CHECK-NEXT to match the relations that are printed immediately after the symbol occurrence.

This comment has been minimized.

@rockbruno

rockbruno Sep 28, 2018

Contributor

Does this rule applies only to AbstractFunctionDecl? It seems that merely mimicking the generic Decl behaviour does the trick:

    auto Parent = getParentDecl();
    if (Parent && isa<AbstractFunctionDecl>(Parent))
      addRelation(Info, (unsigned)SymbolRole::RelationContainedBy, Parent);

module/Swift | SwiftModuleC | [[SwiftModuleC_USR]] | Ref,RelCont | rel: 1
RelCont | instance-method/Swift | myMethod() | [[myMethod_USR:.*]]

@@ -0,0 +1,29 @@
// RUN: %target-swift-ide-test -print-indexed-symbols -enable-source-import -source-filename %s -I %S/Store/Inputs | %FileCheck %s

This comment has been minimized.

@akyrtzi

akyrtzi Sep 28, 2018

Member

It's shaping up great! One final (I think) suggestion about the test case; add a line like this:

import NonExistingModuleName // make sure there's no problem with invalid imports

as per comment this would be to guard that there's no issue occurring with invalid imports, like some crash/assertion hit while trying to index its module reference.

This comment has been minimized.

@rockbruno

rockbruno Sep 28, 2018

Contributor

Makes sense! I added // CHECK-NOT: {{.*}} | NonExistingModuleName to it to guarantee it doesn't even show up, but if it has performance implications we can leave it out.

@akyrtzi

This comment has been minimized.

Member

akyrtzi commented Sep 28, 2018

@swift-ci smoke test

@akyrtzi

This comment has been minimized.

Member

akyrtzi commented Sep 28, 2018

Thanks for your patience and work on this! It will be critically useful for providing global rename functionality for frameworks at some point.

@rockbruno

This comment has been minimized.

Contributor

rockbruno commented Sep 28, 2018

Thank -you- for the tips and assistance! I promise the next PRs will be far less painful...

@akyrtzi

This comment has been minimized.

Member

akyrtzi commented Sep 28, 2018

I think test fails on linux because ClangModuleB.h uses @import ClangModuleA; and ObjC interop does not exist on linux.
Try changing that line to #import "ClangModuleA.h". Also I think this change would eliminate the need to have // REQUIRES: objc_interop in unit-pcm-dependency.swift

@akyrtzi

This comment has been minimized.

Member

akyrtzi commented Sep 28, 2018

Also add // REQUIRES: objc_interop to SourceKit/Indexing/index_constructors.swift.

@rockbruno

This comment has been minimized.

Contributor

rockbruno commented Sep 28, 2018

Done

@akyrtzi

This comment has been minimized.

Member

akyrtzi commented Sep 28, 2018

@swift-ci smoke test

@akyrtzi akyrtzi merged commit bf84b29 into apple:master Sep 28, 2018

2 checks passed

Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment