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

go/doc: add fields to Example struct to indicate type/method/suffix #23864

Closed
dsnet opened this issue Feb 16, 2018 · 18 comments
Closed

go/doc: add fields to Example struct to indicate type/method/suffix #23864

dsnet opened this issue Feb 16, 2018 · 18 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted
Milestone

Comments

@dsnet
Copy link
Member

dsnet commented Feb 16, 2018

The doc package currently provides:

func Examples(files ...*ast.File) []*Example

Given a set of source files, this only parses out the examples as a flat list.

However, the testing package documents a syntax of how to associate examples with:

  • A package (e.g., Example or Example_suffix)
  • A function (e.g., ExampleF or ExampleF_suffix)
  • A type (e.g., ExampleT or ExampleT_suffix)
  • A method (e.g., ExampleT_M or ExampleT_M_suffix)

As the number of tools that provide godoc-like functionality increases, the manual logic to parse these example names and associate it with the appropriate top-level declaration is somewhat tedious and easy to get wrong.

Given that this syntax is documented, I propose that the doc package provide first-class support for doing this association. The additional functionality is as follows:

  • New uses the ast.Package.Files input to produce an internal []Example.
  • An Examples []Example field is added to the Package, Type, and Func types. They are populated as appropriate according to what the relevant examples are associated with.
  • Examples that do not follow the example syntax or reference an non-existent declaration could be either ignored or just associated with the entire package.

\cc @griesemer

@gopherbot gopherbot added this to the Proposal milestone Feb 16, 2018
@dsnet
Copy link
Member Author

dsnet commented Feb 16, 2018

Some evidence that this is an issue: the two most common godoc implementations don't handle this the same way:

  • golang/tools/godoc: The stripExampleSuffix function assumes that the suffix may not contain another '_' character.
  • golang/gddo/doc: The builder.getExamples method assumes that so long as there is a prefix match and the suffix starts with an uppercase, it doesn't care what the rest of the suffix looks like.

Arguably the gddo approach is more correct. The point is that the divergence in behavior is a why the standard library should do this association for the user.

@gopherbot
Copy link

Change https://golang.org/cl/94855 mentions this issue: go/doc: add support for classifying Examples

@rsc
Copy link
Contributor

rsc commented Feb 26, 2018

Why do you want to modify go/doc? Is it just so godoc and gddo can share something?
Also we need not change the accessor. We could provide a helper that just takes the string name and returns info about the parsed form of that string name.

@dsnet
Copy link
Member Author

dsnet commented Feb 26, 2018

Why do you want to modify go/doc?

To have a canonical implementation of the syntax laid on in the testing package. As it currently stands, every godoc-like tool does something slightly different.

Is it just so godoc and gddo can share something?

and the additional godoc-like tools that are springing up.

Also we need not change the accessor. We could provide a helper that just takes the string name and returns info about the parsed form of that string name.

I'm not sure what you mean by that. Are you saying we shouldn't alter the doc.Package struct? Modifying the struct makes it easier to pass doc.Package to html/template or text/template. IMO, the helper functions are harder to work with.

@rsc
Copy link
Contributor

rsc commented Mar 5, 2018

I don't think we expect go/doc structs in their current form to be guaranteed to be helpful for any use of template.

I do think that something like

func ParseExampleName(name string) ExampleInfo

// For "ExampleFoo_M_suffix"
type ExampleInfo struct {
    Name string // "Foo"
    Method string // "M"
    Suffix string // "suffix"
}

could be useful for godoc and cmd/doc and other things to share. That would still give you a canonical implementation of the syntax. Or add a few fields to the doc.Example struct:

Name string // "ExampleFoo_M_suffix" (already in struct)
Base string // "Foo"
Method string // "M"
Suffix string // "suffix"

?

@dsnet
Copy link
Member Author

dsnet commented Mar 5, 2018

That is not sufficient. You need a list of top-level type, function, and method identifiers to disambiguate whether Foo_Bar_baz refers to a method named Foo.Bar or a type named Foo_Bar. It is only a stylistic recommendation that most top-level Go identifiers do not have _ in their name. Thus, classifying by _ delimiter alone is insufficient. The gddo tool does this more correctly except for the worst-case situation where both Foo.Bar and Foo_Bar exists; however that case is rare and impossible to reconcile.

Another ambiguity is whether Foo_bar refers to Foo with a suffix of bar or an actual type named Foo_bar. If we were relying on purely lexical parsing, I would not recommend us adding any logic to go/doc at all since I would still perform this on my own (as it is more correct).

@griesemer griesemer self-assigned this Mar 19, 2018
@rsc
Copy link
Contributor

rsc commented Apr 16, 2018

The ambiguity that @dsnet mentions strikes out the ParseExample func, but it seems like the doc.Example field additions are still fine, since they're returned as part of something that looks at the whole package and knows its types and methods.

@rsc
Copy link
Contributor

rsc commented Apr 23, 2018

OK, it sounds like we should add the fields to the Example struct (the second half of my message from March 5).

And maybe also we should provide the Example slice in the overall Package struct instead of having a second call, although there may be some concerns around opening files during doc.New that were previously not opened (*_test.go).

@dsnet and @griesemer, if you both agree about adding the fields to the Example struct, please mark this proposal-accepted.

@rsc rsc changed the title proposal: go/doc: provide first-class support for classifying examples proposal: go/doc: add fields to Example struct to indicate type/method/suffix Jun 5, 2018
@rsc
Copy link
Contributor

rsc commented Jun 5, 2018

Ping @dsnet and @griesemer, waiting on your acceptance.

@dsnet
Copy link
Member Author

dsnet commented Jun 11, 2018

Spoke to @griesemer, adding fields to the doc.Example struct and extracting top-level declarations from the set of input *ast.File SGTM.

This will require some adjustments to how doc.Examples is called as it seems most usages just pass into only the test files (e.g., here).

And maybe also we should provide the Example slice in the overall Package struct instead of having a second call, although there may be some concerns around opening files during doc.New that were previously not opened (*_test.go).

I support this, but can be a future addition.

@ianlancetaylor ianlancetaylor added Proposal-Accepted NeedsFix The path to resolution is known, but the work has not been done. and removed Proposal labels Jun 18, 2018
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Unplanned Jun 18, 2018
@ianlancetaylor ianlancetaylor changed the title proposal: go/doc: add fields to Example struct to indicate type/method/suffix go/doc: add fields to Example struct to indicate type/method/suffix Jun 18, 2018
@griesemer griesemer removed their assignment Jun 18, 2018
@dsnet
Copy link
Member Author

dsnet commented Sep 2, 2018

#27442 was closed as a duplicate of this. I should note that the unified behavior for example parsing may require an update to the documentation in testing. In particular, the documentation states that the suffix must start with a lowercase letter, however both tools/godoc and gddo both a treat a suffix as anything that isn't an uppercase.

@dmitshur
Copy link
Contributor

dmitshur commented Nov 1, 2019

I should note that the unified behavior for example parsing may require an update to the documentation in testing. In particular, the documentation states that the suffix must start with a lowercase letter, however both tools/godoc and gddo both a treat a suffix as anything that isn't an uppercase.

There is a go vet check for example suffixes that requires a lower case letter as documented in testing:

$ go vet
# example.com/p
./p_test.go:5:1: Example_123Suffix has malformed example suffix: 123Suffix

So I think we can safely update godocs to match what's written in testing documentation as well.

@dmitshur dmitshur assigned dmitshur and unassigned dsnet Nov 1, 2019
@kortschak
Copy link
Contributor

Please take the opportunity to relax that restriction. The suffix used here is for human consumption and is not a Go idenfier label. Making it !uppercase rather than lowercase would be much better.

@dmitshur
Copy link
Contributor

dmitshur commented Nov 1, 2019

Thanks for feedback @kortschak. I've looked over #27442 for the rationale presented more closely and will take it into account.

@dmitshur
Copy link
Contributor

dmitshur commented Nov 4, 2019

I have talked with @dsnet about the status of CL 94855, and he gave me permission to take its code and make the necessary changes to address code review comments and make progress on resolving this issue. (I will be including a note in the commit message of my CL that the code is based on his CL.)

I have addressed the code review comments I left on the original CL (here and here). We've been testing the code in that CL as part of documentation rendering for the discovery site (#33654), which has allowed me to detect a problem with the API it attempted to implement. From its commit message:

The new behavior is as such:

  1. New inspects the ast.Package.Files input for any test source files.
  2. It generates a list of examples by parsing the test files.

It turned out to be problematic to try to provide test source files to the go/doc package via ast.Package.Files for the following reason. ast.NewPackage take a map of Go files that it expects to belong to a single Go package. However, external tests have a "_test" suffix in the package name. That means doing the following:

files := map[string]*ast.File{...} // all .go files, including test source files
apkg, _ := ast.NewPackage(fset, files, ...)
p := doc.New(apkg, ...)

Will result in ast.NewPackage potentially encountering a file with "_test" suffix in the package name first, and will incorrectly dismiss other Go files as having a mismatching package name.

We've used a workaround like this:

apkg, _ := ast.NewPackage(fset, goFiles, ...)
for name, f := range testGoFiles {
	apkg.Files[name] = f
}
p := doc.New(apkg, ...)

But that does not seem to be a good API, and it requires doc.New to do the work of splitting Go files into test and non-test ones.

I tried to come up with the least bad new API to resolve this problem, and I have created a new CL 204830 that implements it in order to resolve this issue. Review is welcome.

@gopherbot
Copy link

Change https://golang.org/cl/204830 mentions this issue: go/doc: add support for classifying examples

@gopherbot
Copy link

Change https://golang.org/cl/217125 mentions this issue: doc/go1.14: mention go/doc.NewFromFiles and associated new data fields

gopherbot pushed a commit that referenced this issue Jan 31, 2020
Updates #23864
Updates #36878

Change-Id: I6efdaafbe5207c625643f201a5931ad735941365
Reviewed-on: https://go-review.googlesource.com/c/go/+/217125
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/218477 mentions this issue: go/doc: clarify that NewFromFiles caller must filter by GOOS/GOARCH

gopherbot pushed a commit that referenced this issue Feb 10, 2020
The most well known and important build constraints to take into
account when rendering package documentation are the GOOS/GOARCH
values. Make it more clear in the NewFromFiles documentation that
they are a part of all build constraints that the caller is
responsible for filtering out.

Also suggest the "go/build".Context.MatchFile method for performing
file matching. The logic to perform build context file matching is
subtle and has many rules that aren't well known (for example,
taking the gc or gccgo compiler into account). It is currently the
only exported API in the standard library that implements this logic,
and it would be unfortunate if people attempt to re-create it because
they don't realize it is already available.

Updates #23864

Change-Id: I3c5901e7081acf79125b2d429ec3aa3b58416ed7
Reviewed-on: https://go-review.googlesource.com/c/go/+/218477
Reviewed-by: Robert Griesemer <gri@golang.org>
@golang golang locked and limited conversation to collaborators Feb 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted
Projects
None yet
Development

No branches or pull requests

7 participants