x/tools/go/packages: add syntax only mode?

[matloob]

Maybe we should add a go/packages LoadMode to just parse files but not perform type checking.

The name LoadSyntax is already taken, so I don't want to change its meaning, but if we can find a good name for the new mode, it seems like we could add it in. One unfortunate detail would be that we'd break the "feature" of the LoadMode heirarchy that each successive mode provides more information.

[myitcv]

cc @heschik @mvdan @rogpeppe

[myitcv]

Also cc @ianthehat because when @mvdan (I think?) proposed this some time ago, there was some rationale against it which I can't recall.

[mvdan]

I'm not sure about the "more information" hierarchy. The increments were designed in a somewhat weird way to the user, where type information can be obtained before syntax trees.

If we can make an exception with the hierarchy, I'd suggest:

LoadFiles   // list files
LoadImports // files and dependencies

LoadTypes      // type info for decls (plus files and deps?)
LoadOnlySyntax // syntax trees with no type info

LoadSyntax    // syntax trees with type information
LoadAllSyntax // syntax trees with type information, recursively

In a way, LoadTypes and LoadOnlySyntax would "branch out", being joined again in LoadSyntax, which contains both syntax trees and type information, but being slower than both of the previous ones.

I'm not suggesting a LoadAllSyntax without type information, because that would break the hierarchy in a second place. I don't think loading packages recursively without type information is going to be as big of a use-case as LoadOnlyTypes. I see LoadOnlySyntax as being closer to LoadImports than Load*Syntax, in that sense.

This is a suggestion on the hierarchy though, not so much the name. Other names that come to mind are LoadSyntaxNoTypes, and LoadSource (since it's the first mode that incurs parsing source files on disk).

[mvdan]

Another way to think about my proposed addition is that it would be go/package's adaptation of go/loader, which had a way to disable (most) type information, but had no way to load all recursive dependencies too. If anyone has the niche use case of loading all recursive dependencies without type information, they can rely on LoadImports like now, or pay the extra runtime cost with LoadAllSyntax.

[mvdan]

If breaking backwards compatibility was an option, then my vote would go for:

LoadFiles   // list files
LoadImports // files and dependencies

LoadTypes  // type info for decls (plus files and deps?)
LoadSyntax // syntax trees with no type info

LoadTypedSyntax    // syntax trees with type information
LoadAllTypedSyntax // syntax trees with type information, recursively

This way, it's much clearer which modes contain types and/or syntax, and by consequence which ones don't.

[mvdan]

Throwing another idea into the pile - perhaps we could add typechecking knobs into packages.Config, much like loader.Config had a types.Config and TypeCheckFuncBodies func(path string) bool.

I'm not sure how much API from go/types we want to expose. A single bool like SkipTypecheck could be enough, making LoadSyntax and LoadAllSyntax only parse syntax trees, skipping type information altogether.

The advantage here is that we wouldn't mess with the load mode hierarchy, and we'd also support recursive loading of syntax trees without type information. The disadvantage would be that LoadMode would now be a bit more subtle, as it would have a separate knob in the config.

[matloob]

@jayconrod who suggested the idea, and @ianthehat for his opinions (and the hat)

We've thought about this a bit and on reflection I think the hierarchy was a mistake. But: we promised compatibility so we're going to keep it. But jayconrod suggested and idea I like:

The proposal would be to add a bit field (or something that behaves like it) that would allow users to specify the options they want (do I want type information? ASTs? etc.) I think some options would have to override others... we'd have to come up with clear precedence rules. We could then make the "iota" mode one lower than LoadFiles, and have that select the bit mask.

What do y'all think?

[mvdan]

We could then make the "iota" mode one lower than LoadFiles, and have that select the bit mask.

Not sure I follow. Could you give a small code snippet to demonstrate?

Overall the idea seems feasible to me, as it would let us add the features and not break backwards compatibility. It would also allow us to add more knobs to the load mode in the future without having this problem again.

Having said that, it seems to me like long-term it would result in a bit of a hacky and complex API. Which might be fine, if we say that most go/packages users should steer away from the lower-level bitfield load mode.

Perhaps the answer could be for go/packages to have this bit field, and reconsider the load mode API for v2/go/packages once we've seen which load mode knobs had to be added to the bit field. Perhaps we can come up with a nice iota hierarchy then.

[rogpeppe]

Thinking about @matloob's suggestion, how about something like this:

// When bitwise-OR'd in a LoadMode value, the WithoutTypes bit
// signifies that type checking should not be done. This enables
// loading syntax without the overhead of type checking. Load
// will return an error if WithoutTypes is combined with a
// LoadMode value that inherently requires type checking, such
// as LoadTypes.
//
// For example, to load syntax trees without type checking, use
// LoadSyntax|WithoutTypes.
WithoutTypes = 1<<8

This potentially allows for more LoadMode values and the idiom might also extend to suppressing other overheads in the future, such the type checking of function bodies.

[jayconrod]

To elaborate on the idea @matloob described, I think tools need to describe exactly what they need from go/packages, and go/packages should gather that information in the cheapest way possible. For example, maybe you just care about syntax trees but don't need type information. Or maybe you need type information for the target package only, and go/packages could give you enough type information about your dependencies by loading export data (which is cached) instead of parsing and type checking (not cached).

So the idea was that LoadMode would be a bit field where each bit corresponds to something the caller specifically needs. Maybe there's a bit for each packages.Package field. Maybe a set of bits for directly named packages, and a separate set of bits for dependencies.

To preserve compatibility in the current API, each current LoadMode constant would be converted to an equivalent bit mask. Existing code wouldn't need to change, and we wouldn't need to do a v2.

[gopherbot]

Change https://golang.org/cl/162140 mentions this issue: go/packages: split LoadMode into fine-grained options

[matloob]

If you have opinions on what the bits should indicate, see the above change...

[matloob]

Hi, I've submitted my change. Writing out the LoadMode in terms of bits and setting LoadSyntax (but not LoadTypes) should only load syntax.

[matloob]

I'll close this issue.