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 · 16 comments
Assignees
Milestone

Comments

@dsnet
Copy link
Member

@dsnet 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
@gopherbot gopherbot added the Proposal label Feb 16, 2018
@dsnet

This comment has been minimized.

Copy link
Member Author

@dsnet 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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 16, 2018

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

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc 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

This comment has been minimized.

Copy link
Member Author

@dsnet 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

This comment has been minimized.

Copy link
Contributor

@rsc 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

This comment has been minimized.

Copy link
Member Author

@dsnet 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

This comment has been minimized.

Copy link
Contributor

@rsc 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

This comment has been minimized.

Copy link
Contributor

@rsc 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

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jun 5, 2018

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

@dsnet

This comment has been minimized.

Copy link
Member Author

@dsnet 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.

@gopherbot gopherbot added the Proposal label Jun 18, 2018
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Unplanned Jun 18, 2018
@gopherbot gopherbot added the Proposal label 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

This comment has been minimized.

Copy link
Member Author

@dsnet 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

This comment has been minimized.

Copy link
Member

@dmitshur 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

This comment has been minimized.

Copy link
Contributor

@kortschak kortschak commented Nov 1, 2019

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

This comment has been minimized.

Copy link
Member

@dmitshur 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

This comment has been minimized.

Copy link
Member

@dmitshur 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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 4, 2019

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

@dmitshur dmitshur modified the milestones: Unplanned, Go1.14 Nov 12, 2019
@gopherbot gopherbot closed this in 4faada9 Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.