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

Jump to bottom

cmd/go,x/tools/gopls: confusing behavior of go mod when go.work is in play #60430

Closed
frankli0324 opened this issue May 25, 2023 · 10 comments
Closed

cmd/go,x/tools/gopls: confusing behavior of go mod when go.work is in play #60430

frankli0324 opened this issue May 25, 2023 · 10 comments
Assignees
@findleyr
Labels
gopls/metadata Issues related to metadata loading in gopls gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
gopls/v0.15.0

Comments

@frankli0324
Copy link

frankli0324 commented May 25, 2023 •
edited

What version of Go are you using (go version)?

$ go version
go version go1.20.4 darwin/amd64

What did you do?

with a go workspace https://github.com/frankli0324/workspace_demo (I committed the go.work file for demonstration purposes)

go.work // uses module_a and module_b
module_a
  - go.mod // that *doesn't* require module_b
  - main.go // imports module_b
module_b
  - go.mod
  - lib.go

module_a can be compiled

now remove go.work, compilation fails with:

main.go:6:2: no required module provides package github.com/frankli0324/workspace_demo/module_b; to add it:
	go get github.com/frankli0324/workspace_demo/module_b

also note that go install github.com/frankli0324/workspace_demo/module_a succeeds, and go install github.com/frankli0324/workspace_demo/module_b@remove-go-work also succeeds.
the remove-go-work branch doesn't have a go.work file and module_a doesn't require module_b

What did you expect to see?/What did you see instead?

I expect either gopls to stop complaining about missing modules in go.mod and "you must go mod tidy" or go build/install fail to build the module if go.mod is incorrectly constructed (missing require)

Conclusion

From what I observe, I believe the essence of go work is forcing submodules to build as if there's an replace $declared_module_name => ./$local_path in each of the used go.mods:

in persudo-code:

used = use_directives_from(go.work)
replacements = []
for mod in used:
    mod_name = get_module_name_from(path.join(mod, go.mod))
    replacements.append(mod, mod_name)
for mod in used:
    for m, mod_name in replacements:
        set_replacement_for(mod, mod_name => m)

However, normally if module_a doesn't require module_b, if module_b is replaced anyway, go build still fails:

main.go:6:2: module github.com/frankli0324/workspace_demo/module_b provides package github.com/frankli0324/workspace_demo/module_b and is replaced but not required; to add it:
	go get github.com/frankli0324/workspace_demo/module_b

this is where I think is inconsistent and erroneous.

I'm aware that #52962 raised the issue already, but I believe it's not a duplicate. Also the issue seems wasted since no one is getting anywhere closer to an answer.
@gopherbot gopherbot added the gopls Issues related to the Go language server, gopls. label May 25, 2023
@frankli0324
Copy link
Author

frankli0324 commented May 25, 2023 •
edited

actually this also raises a question: how should versions be managed if we could build the module_a without requiring module_b? or is this "undefined behavior"? Is this some legacy from the GOPATH era?

@suzmue suzmue added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 25, 2023
@frankli0324
Copy link
Author

also related: #53502

@jamalc jamalc assigned jamalc and findleyr and unassigned jamalc Jun 1, 2023
@findleyr findleyr changed the title cmd/go|tools/gopls: confusing behavior of go mod when go.work is in play cmd/go|x/tools/gopls: confusing behavior of go mod when go.work is in play Jul 6, 2023
@findleyr findleyr added this to the gopls/v0.13.0 milestone Jul 6, 2023
@findleyr findleyr removed their assignment Jul 6, 2023
@findleyr
Copy link
Contributor

findleyr commented Jul 6, 2023

Slotted this for gopls@v0.13.0, when we are revisiting UX of modules/workspaces/etc.

@seankhliao seankhliao changed the title cmd/go|x/tools/gopls: confusing behavior of go mod when go.work is in play cmd/go,x/tools/gopls: confusing behavior of go mod when go.work is in play Jul 6, 2023
@adonovan adonovan added the gopls/metadata Issues related to metadata loading in gopls label Aug 31, 2023
@findleyr findleyr modified the milestones: gopls/v0.14.0, gopls/v0.15.0 Oct 9, 2023
@findleyr
Copy link
Contributor

findleyr commented Jan 6, 2024

This should have been fixed by #57979. Now gopls should just do the right thing when you open a Go file, auto-detecting the applicable workspace. Please comment if that is not the case.

@findleyr findleyr closed this as completed Jan 6, 2024
@findleyr findleyr self-assigned this Jan 6, 2024
@frankli0324
Copy link
Author

@findleyr so the behavior of go build and go install is expected? may I ask for an explanation?

@findleyr
Copy link
Contributor

findleyr commented Jan 8, 2024

@frankli0324 sorry, let me make sure I understand: you are asking about the behavior of go build with/without a go.work file?

In general, go.work files are only used for development: for example they are not used by go install. That's why go.mod files must require all the modules they use. I can see an argument for making it an error for module_a to import module_b, even if a go.work file uses both, but apparently that's not how it works (probably for simplicity). I don't see a strong reason to allow unused replace directives, so it seems correct to me that this is an error.

CC @bcmills @matloob for the go.work behavior, though as far as I understand it, this is working as intended.

I agree that the behavior of gopls was confusing, since gopls should be able to figure out how to build your workspace with and without a go.work file. That is what was fixed by #57979.

@matloob
Copy link
Contributor

matloob commented Jan 8, 2024

Yes, from what I've read it seems like it's working as intended. go.work will mask missing modules errors but each module should have the proper requirements to be able to build without a go.work file.

@frankli0324
Copy link
Author

frankli0324 commented Jan 9, 2024 •
edited

however this(wrong branch previously, updated) builds without errors with or without a go.work @matloob .
It doesn't have a go.work but builds successfully. module_a doesn't require module_b while importing it, I'm expecting an error for this case.

➜  ~ go install github.com/frankli0324/workspace_demo/module_a@remove-go-work
go: downloading github.com/frankli0324/workspace_demo v0.0.0-20230525143326-5f566b988c3e
go: downloading github.com/frankli0324/workspace_demo/module_a v0.0.0-20230525143326-5f566b988c3e
go: finding module for package github.com/frankli0324/workspace_demo/module_b
go: downloading github.com/frankli0324/workspace_demo/module_b v0.0.0-20230525151631-f91535a39128
go: downloading github.com/frankli0324/workspace_demo v0.0.0-20230525151631-f91535a39128
go: found github.com/frankli0324/workspace_demo/module_b in github.com/frankli0324/workspace_demo/module_b v0.0.0-20230525151631-f91535a39128
➜  ~ module_a
test

So this is a seperate issue then?

@frankli0324
Copy link
Author

frankli0324 commented Jan 9, 2024 •
edited

also, as a matter of fact, CI actions and workflows build the release builds with go build. go build respects go.work by default, go install doesn't. I think this is why many mistook GOWORK for prod purposes.
So is it expected to GOWORK=off before packaging? sounds weird, if go.work is used for dev purposes only, isn't it more reasonable to disable it by default, and users should turn them on in dev environments, similar to how we configure other GOXX variables?

@findleyr
Copy link
Contributor

findleyr commented Jan 9, 2024

See also #53502. We could have clearer documentation about best practices for go.work, such as whether they should be checked in to VCS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@findleyr findleyr

Labels
gopls/metadata Issues related to metadata loading in gopls gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
gopls/v0.15.0
Development

No branches or pull requests
7 participants
@adonovan @suzmue @gopherbot @matloob @jamalc @frankli0324 @findleyr