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

[SourceKit] Report cursor info information for literals #67613

Merged
merged 10 commits into from Aug 9, 2023

Conversation

barinsim
Copy link
Contributor

Adds literals support for the CursorInfo request.
Previously CursorInfo just returned error for literals.
Resolves #57725

Adds literals support for the CursorInfo request.
Previously CursorInfo just returned error for literals.
Implements issue apple#57725
Copy link
Contributor

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR, @barinsim. I’ve got a couple of minor comments inline. On a more general level, I think the problem you’ve been facing is that all of cursor info is designed to always reference a declaration, if you’re doing cursor info on a function call, we report the function declaration or if you’re doing cursor info on a variable, we return the corresponding variable declaration.

For literals, there isn’t such a natural declaration and that’s why you needed to add a LitExpr to DeclInfo, which doesn’t really make sense since the information about a declaration (that’s what DeclInfo is), shouldn’t need an expression. And that’s also why you needed the isLiteral checks.

What I would suggest is that we report the initializer that actually constructs eg. a Double from the literal (Double.init(_builtinIntegerLiteral:)) in cursor info. That initializer returns a value of type Double so you shouldn’t need all the special isLiteral handling anymore. You should be able to get that initializer using getBuiltinInitializer on BuiltinLiteralExpr.

Let me know if you need help with anything or have any questions about my suggestion or in general.


And one small thing: Could you run git-clang-format before committing your sources to make sure they are formatted correctly?

test/SourceKit/CursorInfo/cursor_info.swift Outdated Show resolved Hide resolved
test/SourceKit/lit.local.cfg Outdated Show resolved Hide resolved
@barinsim
Copy link
Contributor Author

barinsim commented Aug 1, 2023

@ahoppen Thank you very much for your feedback! Reporting the actual initializer definitely fits better 🙂 I hope I addressed everything.

2 quick notes:

  • I preferred the getInitializer() and use getBuiltinInitializer() as a fallback, because I would argue in this case:
struct A: ExpressibleByIntegerLiteral {
    init(integerLiteral value: Int) {
        self.value = value
    }
    let value: Int
}
var a: A = 42

for 42 we want to report the user defined init instead of the builtin one.

  • The node for the nil literal does not contain any initializer info. I am not sure why nor if it is expected. For now, I removed the 2 testcases testing the nil

@barinsim barinsim requested a review from ahoppen August 1, 2023 15:25
@ahoppen
Copy link
Contributor

ahoppen commented Aug 1, 2023

  • I preferred the getInitializer() and use getBuiltinInitializer() as a fallback, because I would argue in this case:
struct A: ExpressibleByIntegerLiteral {
    init(integerLiteral value: Int) {
        self.value = value
    }
    let value: Int
}
var a: A = 42

for 42 we want to report the user defined init instead of the builtin one.

Oh, that’s even better. I didn’t think to look for getInitializer after I found getBuiltinInitializer. Good catch 👍🏽

  • The node for the nil literal does not contain any initializer info. I am not sure why nor if it is expected. For now, I removed the 2 testcases testing the nil

Cursor info still works on nil if you have a custom type that conforms to ExpressibleByNilLiteral, e.g.

struct MyOptional: ExpressibleByNilLiteral {
  init(nilLiteral: ()) {}
}
let _: MyOptional = nil

The issue why we don’t get cursor info when nil is used for Optional is that the initializer doesn’t get set here

swift/lib/Sema/CSApply.cpp

Lines 2628 to 2634 in 0591975

// By far the most common 'nil' literal is for Optional<T>.none.
// We don't have to look up the witness in this case since SILGen
// knows how to lower it directly.
if (auto objectType = type->getOptionalObjectType()) {
cs.setType(expr, type);
return expr;
}

If you wanted to, you could look into setting the initializer here as well but I’m not entirely sure what the trade-off is between performance of looking up Optional.init(nilLiteral:) vs the value you get out of cursor info on these nil literals. In either case, I think this change shouldn’t be part of this PR.

@barinsim barinsim requested a review from ahoppen August 2, 2023 22:08
Copy link
Contributor

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Looks very good. I just have a few minor comments.

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp Outdated Show resolved Hide resolved
tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp Outdated Show resolved Hide resolved
tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp Outdated Show resolved Hide resolved
@barinsim barinsim requested a review from ahoppen August 3, 2023 07:37
@barinsim
Copy link
Contributor Author

barinsim commented Aug 7, 2023

@ahoppen Thank you very much for iterating on this with me! 🙂 I addressed the last changes you proposed. If they look alright to you, could you please run the CI?

Copy link
Contributor

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thank you @barinsim. Looks great to me

@ahoppen
Copy link
Contributor

ahoppen commented Aug 8, 2023

@swift-ci Please test

Comment on lines 351 to 352
CursorInfo = new ResolvedExprStartCursorInfo(CursorInfo->getSourceFile(),
CursorInfo->getLoc(), E);
Copy link
Contributor

Choose a reason for hiding this comment

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

This change breaks test/refactoring/RefactoringKind/basic.swift:

When you have an interpolated string literal and perform cursor info on it, previously we got an interpolated string literal expression. With this change, we get the string literal expression of the first segment inside the string literal before the first interpolation and thus offer to localize the string, which we shouldn’t. I checked and it looks like this change is no longer necessary to get cursor info for literals. Could you revert it?

Copy link
Contributor Author

@barinsim barinsim Aug 8, 2023

Choose a reason for hiding this comment

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

@ahoppen Yes, you are right 👍 I added a test case into the CursorInfo suite as well.

During which I found that getting info of the expression segment in the interpolated string does not work. We consider the whole interpolated string as a single token which has nested tokens within the expression segments. For example this causes that getSourceRange() for ValueDecl returns such range:

<START>var foo = <END>"Hello \(42) world"

This does not work if you are looking for the node of the 42. Converting to a char based range fixed this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, the good old char source range conversion for interpolated string literals. I so wish this wouldn’t be necessary. 🙄 Good catch though.

@barinsim barinsim requested a review from ahoppen August 8, 2023 21:56
@ahoppen
Copy link
Contributor

ahoppen commented Aug 8, 2023

@swift-ci Please smoke test

@ahoppen ahoppen merged commit bba2f47 into apple:main Aug 9, 2023
3 checks passed
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.

[SR-15411] Report cursor info information for literals
2 participants