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

x/tools/go/packages: Package.Fset is nil when using NeedSyntax #48226

Open
gordonklaus opened this issue Sep 7, 2021 · 4 comments
Open

x/tools/go/packages: Package.Fset is nil when using NeedSyntax #48226

gordonklaus opened this issue Sep 7, 2021 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@gordonklaus
Copy link
Contributor

What did you do?

package main

import (
	"fmt"
	"golang.org/x/tools/go/packages"
)

func main() {
	conf := &packages.Config{
		Mode: packages.NeedSyntax,
	}
	pkgs, err := packages.Load(conf, "fmt")
	if err != nil {
		panic(err)
	}
	fmt.Println(pkgs[0].Fset == nil)
}

What did you expect to see?

false

What did you see instead?

true

Rationale

This is apparently working as intended, as the documentation reads:

	// Fset provides position information for Types, TypesInfo, and Syntax.
	// It is set only when Types is set.
	Fset *token.FileSet

However, I see no reason why Fset should not be exposed when NeedSyntax is requested. In particular, Fset is needed for looking up position information for syntax elements.
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 7, 2021
@gopherbot gopherbot added this to the Unreleased milestone Sep 7, 2021
@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 7, 2021
@thanm
Copy link
Contributor

thanm commented Sep 7, 2021

@matloob per owners

@adonovan
Copy link
Member

I agree that this is a bug.

In my experience, the packages.Config.Mode API is just hateful. One is forced to choose between the compound Load... modes, whose names are misleading, whose use triggers a deprecation warning in the IDE, and whose current incremental and undocumented declarations do not aid comprehension at all; or the fine-grained Need... modes, which are riddled with bugs such as this one due to the explosion of combinations in the implementation, and which by design invite bugs in other code because of the exponential number of possible "flavors" of Package that result.

Every time I use Mode, I try to do the right thing and specify just the bits I need, and then realize through painful trial and error that I need to enable some extra bit such as NeedTypes just to get access to the FileSet, defeating any possible benefit of the fine-grained interface. Apologies for my part in getting us into this mess.

We should fix the bugs in the fine-grained implementation, but more importantly I think we should un-deprecate the compound modes, creating new ones as needed with appropriate names for the major use cases, and document exactly what every existing mode is good for.

@dominikh
Copy link
Member

Another fun interaction of modes: without NeedModule, go/types.Config.GoVersion won't be set. Which is slightly surprising, because NeedModule is documented as populating the Module field, not as being relevant to type-checking.

@adonovan
Copy link
Member

adonovan commented Feb 20, 2024 •
edited

Similar problems with mode bits:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@dominikh @gordonklaus @adonovan @gopherbot @thanm