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

[CursorInfo] Always add module name to response #39851

Merged
merged 1 commit into from Oct 27, 2021

Conversation

bnbarham
Copy link
Contributor

@bnbarham bnbarham commented Oct 21, 2021

To simplify clients, have the cursorinfo result be consistent whether
requesting a symbol within the current module or not, ie. do not skip
adding the module name.

Resolves rdar://77003299

@bnbarham
Copy link
Contributor Author

@swift-ci please test

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - a006806d746e2966ed0c359dae4dfb6e7ea8a148

@swift-ci
Copy link
Collaborator

Build failed
Swift Test Linux Platform
Git Sha - a006806d746e2966ed0c359dae4dfb6e7ea8a148

@johnfairh
Copy link
Contributor

What's the problem being solved here? Non-Apple folk can't read rdars.

The current design point of omitting the module name for declarations from the module being queried (the IgnoreModule part) is very long-standing and changing the semantics of the API like this will cause some breakage.

@bnbarham
Copy link
Contributor Author

What's the problem being solved here? Non-Apple folk can't read rdars.

The current design point of omitting the module name for declarations from the module being queried (the IgnoreModule part) is very long-standing and changing the semantics of the API like this will cause some breakage.

We'd like the API to be consistent to simplify clients and we believe it's worth the change in behaviour here. Is there a particular client that you know this will break?

@bnbarham
Copy link
Contributor Author

@swift-ci please test

To simplify clients, have the cursorinfo result be consistent whether
requesting a symbol within the current module or not, ie. do not skip
adding the module name.

Resolves rdar://77003299
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@johnfairh
Copy link
Contributor

Well I'll fix up the clients I look after - just wanted to make sure you were aware that this is a breaking change.

@bnbarham
Copy link
Contributor Author

Well I'll fix up the clients I look after - just wanted to make sure you were aware that this is a breaking change.

I'm interested into what sort of breakage emitting the module name causes - is there any more details you can give there? Specifically, I'd like to make sure there's an alternative if we do go ahead with the change.

@johnfairh
Copy link
Contributor

In jazzy, one example is figuring out whether a decl that is missing doc comments is from the user's module or not (ie. related to an extension of a type from somewhere else). The code that does this today does not know [has not needed to know] the name of the user's module -- so I imagine the change is 'just' plumbing this through, as I expect will be the same anywhere else.

@bnbarham
Copy link
Contributor Author

Yeah, plumbing it through seems reasonable for clients that need to differentiate on current vs other modules. Thanks for the details!

@bnbarham bnbarham merged commit 157c742 into apple:main Oct 27, 2021
@bnbarham bnbarham deleted the always-add-module-name branch October 27, 2021 23:06
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.

None yet

4 participants