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

cmd/go: print file name when showing an import cycle #66078

Closed
purpleidea opened this issue Mar 3, 2024 · 20 comments
Closed

cmd/go: print file name when showing an import cycle #66078

purpleidea opened this issue Mar 3, 2024 · 20 comments
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@purpleidea
Copy link

We've all hit the common:

package foo
	imports whatever1
	imports whatever2
	imports whatever3
	imports bar: import cycle not allowed

It would great if it showed the actual file name of the imports, so that when there are multiple files in a package, it's easier to narrow down which one is the offender.

Eg:

package foo: blah.go
	imports whatever1: file1.go
	imports whatever2: something.go
	imports whatever3: thing.go
	imports bar: import cycle not allowed

Of course a more symmetrical approach might look like:

import cycle not allowed
imports foo: blah.go
imports whatever1: file1.go
imports whatever2: something.go
imports whatever3: thing.go
imports package foo

But whatever the formatting the code idea is that we make it easier for the user to know which file was part of the loop.

What about the situation of more than one file offending? Well this is a topological sort, so we already only guarantee that we print a single loop, even when there are more than one paths.

Of note, we don't want a full absolute path printed, we already know where things should be based on package name, we do care about the relative file path in that package.

Thanks!

@seankhliao seankhliao changed the title Print file name when showing an import cycle cmd/go: print file name when showing an import cycle Mar 3, 2024
@mknyszek mknyszek added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest Issues asking for a new feature that does not need a proposal. GoCommand cmd/go labels Mar 4, 2024
@mknyszek mknyszek added this to the Backlog milestone Mar 4, 2024
@mknyszek
Copy link
Contributor

mknyszek commented Mar 4, 2024

CC @bcmills @matloob

@bcmills
Copy link
Contributor

bcmills commented Mar 5, 2024

As a minor nit, I think it would be slightly clearer like imports whatever1 (in file1.go), rather than just a :.

@purpleidea
Copy link
Author

As a minor nit, I think it would be slightly clearer like imports whatever1 (in file1.go), rather than just a :.

I don't think it's a nit, any suggestions to improve the formatting are appreciated! I don't have a good recommendation or a strong preference here. I mostly just care that the data is exposed to the user =D

One suggestion: make it easy for terminal users to copy paste out the filename. On the end is easy for most terminal emulators for example. Other ways are possible too.

@matloob
Copy link
Contributor

matloob commented Jul 15, 2024

@xin-tsla sent a CL implementing something similar to this. The big difference in their CL is that it used full absolute paths with line numbers. That would make jumping to the import in an editor easier, but I agree with @purpleidea that we shouldn't print the full absolute paths. I don't have a strong opinion on the line numbers, but they're not as useful if we don't have absolute paths. It would make finding the import easier, but it's usually not very hard to find an import in the top of a source file.

@xin-tsla
Copy link
Contributor

xin-tsla commented Jul 16, 2024

Hi All, in the CL(https://go-review.googlesource.com/c/go/+/597035), we have the basename in the stack, the error message looks like following:

package cyclic-import-example
        imports cyclic-import-example/packageA from main.go:4:5
        imports cyclic-import-example/packageB from a.go:5:2
        imports cyclic-import-example/packageA from bb.go:5:2: import cycle not allowed
image

In my IDE, it somehow supports to jump to file location even it is a base name.
Just would like to hear everyone's thoughts about the change.
Thank you!

@purpleidea
Copy link
Author

In my IDE, it somehow supports to jump to file location even it is a base name.

I don't think including the line numbers is helpful since 99% of the time all the imports are all at the top anyways, and if not they'd be easy to find.

The downside however is that those paths are not directly copy+pasteable into a terminal, so this is less ergonomic. The IDE's can handle not having the line numbers.

If we really want to do this properly, then we should use terminal hyperlinks, like so: https://purpleidea.com/blog/2018/06/29/hyperlinks-in-gnome-terminal/

@xin-tsla
Copy link
Contributor

Thanks for the info! @purpleidea
Will read the link to see if we could apply it to the CL.

@xin-tsla
Copy link
Contributor

xin-tsla commented Jul 17, 2024

@purpleidea , it is a good blog to read, thanks for writing and sharing the knowledge! Good to know the hyperlink and escape sequences chars on terminal.
I thought about how to embed escape sequences chars to the code, but I feel like there may be some considerations:

  1. If an user uses a terminal which does not support escape sequences chars, it may show the escape sequences chars mixed with the error messages
  2. Same consideration for windows OS terminal.
  3. It is simpler if we just implement file names.
    But welcome any suggestions/comments from anyone.

As your point in the comment, it makes sense to have the file name without linenumber.
And I made a change, the new error message is:
image
It is close to what you proposed.
Let's list the points in the proposal:

  1. Add file name append each imports in the stack
  2. If the number of file names is more than one for one import, we append with delimiter ,, and the limit is 10 (TBD, it could be less), example in the following screenshot:
image

Wondering are we all in alignment on the proposal? cc: @purpleidea @matloob @mknyszek @bcmills
Please free feel to add comments.
Thanks!

@matloob
Copy link
Contributor

matloob commented Jul 17, 2024

I think this looks reasonable but I think it's enough to list just one file per import

@xin-tsla
Copy link
Contributor

I think this looks reasonable but I think it's enough to list just one file per import

@matloob thanks for the comment!
Addresed and submitted:
image

@matloob
Copy link
Contributor

matloob commented Jul 17, 2024

So I don't think we need the "and more" either. The import cycle error is just showing one path that contains a cycle, but there could be more, so i think that it's implicitly clear that we're just showing one route.

@xin-tsla
Copy link
Contributor

So I don't think we need the "and more" either. The import cycle error is just showing one path that contains a cycle, but there could be more, so i think that it's implicitly clear that we're just showing one route.

@matloob got it. Addressed the comment and submitted:
image

@xin-tsla
Copy link
Contributor

xin-tsla commented Jul 18, 2024

Hi @matloob , @purpleidea ,
Wondering do you think we could finalize the proposal based on the above discussion?
Thanks! cc: @bcmills @mknyszek

@matloob
Copy link
Contributor

matloob commented Jul 23, 2024

That sounds good to me.

@xin-tsla
Copy link
Contributor

@matloob , great! Thanks for the info! 👍
Wondering how do we change the proposal issue to finalized issue or do we have a label for the finalized issue?

@matloob matloob removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 24, 2024
@matloob
Copy link
Contributor

matloob commented Jul 24, 2024

I don't think we need a label. Let's proceed with the review

@dmitshur
Copy link
Contributor

@matloob https://go.dev/wiki/HandlingIssues suggests issues should have one of the Needs{Investigation,Decision,Fix} labels after they're triaged. Removing NeedsInvestigation without adding something else puts it back in the untriaged queue.

NeedsFix seems like the best fit, but feel free to adjust. Thanks.

@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 25, 2024
@matloob
Copy link
Contributor

matloob commented Jul 25, 2024

Oh thanks- NeedsFix is correct.

@xin-tsla
Copy link
Contributor

Thanks all for helping to finalize the proposal!!

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/597035 mentions this issue: cmd/go: add file names for cyclic import error

@dmitshur dmitshur modified the milestones: Backlog, Go1.24 Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants