-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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
x/tools/gopls: rename: modify [pkg.Symbol] doc links in non-doc comments too #57559
Comments
Thanks, this is a good point. We should extend our identifier detection to include the new doc link format. |
Because DocLink renaming is not there yet (the present issue), this comment presents another motivation and a partial workaround for a use case that arises when DocLink is used for data persistence, when go code is used to store data. The repo code for this use case, including the workaround, is https://github.com/fullstack-lang/issuerenaming. Data persisted as go codeIt seems counter intuitive to store data as go code but there is a use case where the In an example for this use case, we have three artifacts:
Now, our client says " Now, let's say that instead of persisting data in json, we store our data as go code and develop a generic AST parser to read the data. The data file in go contains declarations lines like: __Foo__000000_A := (&models.Foo{Name: `A`}) This new persistence approach makes the refactoring simpler because, in the first renaming step from __Foo__000000_A := (&models.Bar{Name: `A`}) We save the json refactoring step. We save (boring) minutes. UML diagram persisted as go codeBypassing the json is economical but now, we would like to also bypass the UML refactoring step. We would like to have the UML diagram data refactored by In the go code describing the UML diagram, we need to say that our class shape that represents the shape.ref = "models.Foo" our renaming step is not automatic. "models.Foo" wont be renamed into "models.Bar" when Enters go 1.19 and DocLinkWith the DocLink syntax such as Technically, this can be done with a comment directive followed by a DocLink item. The go 1.19 //gong:ident [models.Foo]
shape.ref = "" We can ask the generic AST parser to assign the DocLink text to the string value whenever it meets this kind of comment followed by a string assignment (note that After the fix of the present issue, gopls will perform the renaming request of //gong:ident [models.Bar]
shape.ref = "" And the AST parser will assign the correct new value to the shape. Conclusion, the refactoring of the three artifacts takes less that a minute, a nice productivity gain. A partial workaround to the lack of DocLink renamingWe have seen the present issue impacts a use case. To overcome this limitation, we can develop a workaround. Technically, when marshalling the UML diagram, the workaround automatically generates a map of reference to identifiers to identifiers. The identifiers are instances of type var map_DocLink_Identifier map[string]any = map[string]any{
"models.Foo": &(models.Foo{})., When gopls will perform the renaming request, it will change the above code to var map_DocLink_Identifier map[string]any = map[string]any{
"models.Foo": &(models.Bar{})., The Ast Parser has to parse this map and later understands that each time it meets a The workaround is hair splitting because some identifiers are more tricky to express as an Here is the resulting UML diagram after renaming The workaround does not apply to comment like "note"The workaround is partial because in some cases, the reference to the identifier (the DocLink) is not in the UML code but in the package itself. For instance: // GONGNOTE(Note): [models.Foo] is
// related to [models.Waldo] throughs the field
// [models.Foo.Waldos]
const Note = "" This note, a syntax authorized by go, can be translated to a UML note with UML note links but the workaround cannot apply to this because "[Foo]" is in the source package. Is there a need for a
|
Summarizing my understanding of the previous long note: you want to use comments to represent tool-readable relationships between declarations, but renaming doesn't (in general) apply to the contents of comments, so the relationships become stale. The problem is essentially the same as that of writing annotations for static checkers: you can do it with comments, but they quickly become stale. It's better to use Go syntax so that symbol references are first class. For example, when using a hypothetical lock-checking static analysis tool, you could express the annotation "mutex mu guards field f" as a comment:
but it would be more robust to use a Go declaration, for example:
See https://go.dev/cl/489835 for a rough sketch of this approach. |
Your understanding is correct.
I agree the issue is for associations to declarations that become stale after refactoring. However, I have looked at https://go.dev/cl/489835 and I must admit that I am not sufficiently a go expert to understand it. For instance, I do not understand if the solution is for associations within comments or within a go declaration. If I may formulate the requirements for the solution :
|
The main idea in the CL I linked to is: it should be a requirement that annotations not be in comments precisely because this makes them stale, and hard to discover as ordinary references. |
Ok, I understands the CL better now. Unfortunatly, it won't solve the issue of stale DocLink because DocLink are in comments. I did not surmise it was hard to discover them. I am OK if something else than DocLink is available in comments for referencing declarations. |
This appears to a be a dup of #64495, which is fixed, and I verified that renaming a symbol renames (at least some) doc links that refer to it. |
Hi @adonovan , I respectfully disagree, unless mistaken, #64495 (or gops v0.15.3) fixes doc links for functions but does not fixes doc links for structs.
Above code is available at https://github.com/thomaspeugeot/issue_rename_57559 |
I just tested // The Foo type is used with [Bar].
type Foo struct {}
// func Bar uses [Foo].
func Bar(Foo) {} and was able to rename both Foo and Bar, and observe both the ordinary and the doc-link (bracketed) references change consistently. |
Indeed, it works in the above case, but it does not work when
Screen.Recording.2024-06-05.at.08.14.16.mov |
A free-standing comment is not a doc comment: it never appears in |
Ouh la la...., I had never realized that there was two classes of comments, the doc comment and the free standing comments. Sorry to bother but why not subject doc-like link in free-standing comments to gopls' renaming ? Those comments are not exported by |
Because they are not doc comments, as defined by the first line of https://golang.org/doc/comment, so they are not subjected to the same rules by tools such as gofmt, go/doc/comment, and gopls. |
@adonovan it's worth noting that doc links in freestanding comments do work when using go-to-definition, and I rely on that on a weekly basis for e.g. freestanding notes that document quirks about the code. I hope that go-to-definition working in those cases is not by accident, and I would have assumed that renaming would also handle them for the sake of consistency, given that gopls does seem to understand they are doc links in some way. |
@adonovan , BTW, renaming of field does not seem to work. Screen.Recording.2024-06-05.at.23.25.05.mov |
Again, working as intended: doc links never refer to fields, only to packages, package-level declarations, and methods. |
After discussion, reopening as a narrower request to rename doc links in non-doc comments too (by analogy with Hover and Documentation, which support doc links in non-doc comments). |
Doc Links has been added to the go in 1.19 and it is accessible in the AST. Enabling refactoring of DocLinks will improve the maintainability of documentation.
What version of Go are you using (
go version
)?What did you do?
Refactor "Foo" to "Bar"
What did you expect to see?
What did you see instead?
The text was updated successfully, but these errors were encountered: