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

fix multiple time imports using reference variables #663

Merged
merged 6 commits into from
Jan 31, 2023

Conversation

pagran
Copy link
Member

@pagran pagran commented Jan 31, 2023

Alternate realization of fix.

Сurrent version redesigns logic for handling unnecessary (after shrink runtime or literals).
Now instead of renaming unnecessary imports, it creates global unnamed variables referencing to declaration inside unused import (note: go/types returns sorted decl's)

If decl is constant, variable or function:

var _ = <target decl>

If decl is type (struct, type, interface, etc...):

var _ <target decl>

Fix: #658

@pagran
Copy link
Member Author

pagran commented Jan 31, 2023

Referencing to types is not safe (generics, type constraint). How about referring to garbleActionID (making it exportable first)? @mvdan

upd: At second look this idea is not so good, for example unsafe package does not have garbleActionId
upd: For now, i add checking for unsafe declarations, but it is still possible in extreme cases (in theory, if generics appear at runtime)
upd2: It's harder than I thought

@mvdan
Copy link
Member

mvdan commented Jan 31, 2023

Right, you could always have a package which doesn't export any names, if it were imported like _ "pkg/path" for example. And in other cases, I agree that generics can make it awkward.

FWIW I think any exported name works except generic types. For regular types, use var _ pkg.Name. For anything else (funcs, vars, consts), use var _ = pkg.Name. If any package doesn't have any of these, we can for now assume that it won't become unused, which is probably right. A package only becomes unused with -literals when it has a constant being used that we replace, for example.

@mvdan
Copy link
Member

mvdan commented Jan 31, 2023

Relying on garbleActionID is also clever, but I don't think it's a good idea in general. It relies on us being able to modify all packages to insert the var, and inserting the var has always been a hack that I'd like to get rid of at some point.

Copy link
Member

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

Looks good - some thoughts to polish this up.

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@pagran pagran requested a review from mvdan January 31, 2023 18:06
@mvdan mvdan merged commit 7c50979 into burrowers:master Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

-literals can still cause unused import errors
2 participants