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

x/pkgsite, x/tools/cmd/godoc: some code examples are not displayed (but work at godoc.org) #40172

Closed
vmihailenco opened this issue Jul 12, 2020 · 12 comments

Comments

@vmihailenco
Copy link
Contributor

@vmihailenco vmihailenco commented Jul 12, 2020

For example

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Jul 13, 2020

Thanks for reporting.

I tested this package at commit go-pg/pg@6fe164e with pkg.go.dev, godoc.org, and x/tools/cmd/godoc, and can confirm the issue is present in pkg.go.dev and x/tools/cmd/godoc, but not godoc.org.

It's also helpful to look at the complete listing of examples at https://godoc.org/github.com/go-pg/pg?tab=doc#pkg-examples, https://pkg.go.dev/github.com/go-pg/pg/v10?tab=doc#pkg-examples, and the output of x/tools/cmd/godoc:

image

This looks like a valid issue. It needs investigation to understand why these examples, which appear to be following the example naming convention correctly, are not showing up in 2 out of 3 documentation viewers. It might be a problem in the example matching logic in go/doc package, or at a higher level. Once we understand why this is happening, it'll be easier to figure out how to fix this.

/cc @julieqiu @shaqque

@dmitshur dmitshur changed the title x/pkgsite: some code examples are not displayed (but work at godoc.org) x/pkgsite, x/tools/cmd/godoc: some code examples are not displayed (but work at godoc.org) Jul 13, 2020
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Jul 13, 2020

One lead to investigate is that the exported DB type is defined as:

type DB struct {
	*baseDB
	ctx context.Context
}

Notably, baseDB is an embedded field. The DB type gets many of its methods indirectly via that embedded baseDB type:

// Begin starts a transaction. Most callers should use RunInTransaction instead.
func (db *baseDB) Begin() (*Tx, error)

The example classification logic in go/doc.NewFromFiles needs to know the complete API to determine if an example should be attached to a given identifier, so if it's not able to handle methods via embedded types, that could be the cause for this.

I haven't confirmed further, just wanted to share this.

@julieqiu julieqiu added the pkgsite label Jul 13, 2020
@shaqque
Copy link
Contributor

@shaqque shaqque commented Jul 13, 2020

@dmitshur you're right that the issue is related to the embedded field. However, this appears to be expected behavior and is documented in https://github.com/golang/go/blob/master/src/go/doc/example.go#L489-L491

if !token.IsExported(m.Name) || m.Level != 0 { // avoid forwarded methods from embedding
	continue
}

Removing

|| m.Level != 0

appears to fix this issue.

The question now is whether we should consider embedded field methods for examples. One problem I can think of is that the embedded field type could also be exported, resulting in duplicate examples. However, we can check for this case easily (i.e. whether the embedded field is exported) and choose not to show the example for either the original struct type or the embedded field type.

/cc @dsnet @griesemer

@vmihailenco
Copy link
Contributor Author

@vmihailenco vmihailenco commented Aug 15, 2020

Gentle ping.

It seems to be better having duplicated examples rather than not having them at all. Also if duplicates are problematic - authors can rework examples and place them on different struct.

@jba
Copy link
Contributor

@jba jba commented Aug 18, 2020

choose not to show the example for either the original struct type or the embedded field type.

The type for which to show the example is embedded in its name: ExampleT_M should be shown on type T.

In determining whether ExampleT_M applies to T, we should consider all methods of T and *T, including those arising from embedding.

@jba
Copy link
Contributor

@jba jba commented Aug 19, 2020

The fundamental problem is that we don't have full type information when building the doc, so we can't really compute the full method set in all cases.

However, we do know about types in the same package. So I can't think of a reason we shouldn't use that information. So we should remove the m.Level != 0 check in pkgsite's godoc.

It is out of scope for this issue to make the change in go/doc itself.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Aug 19, 2020

So we should remove the m.Level != 0 check in pkgsite's godoc.

It is out of scope for this issue to make the change in go/doc itself.

@jba The original plan for pkgsite's copied go/doc package was to delete it as soon as possible, when pkgsite could start using Go 1.14 in deployment. If you're considering changing that plan, please take into account and update the package comment here.

@jba
Copy link
Contributor

@jba jba commented Aug 20, 2020

@dmitshur I see, I hadn't realized that. In that case, I think we should make the change in go/doc as well.

@jba
Copy link
Contributor

@jba jba commented Aug 20, 2020

I sent https://golang.org/cl/249557 to change go/doc.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 20, 2020

Change https://golang.org/cl/249638 mentions this issue: internal/fetch/internal/doc: support examples on methods from embedded unexported types

gopherbot pushed a commit to golang/pkgsite that referenced this issue Aug 20, 2020
…d unexported types

This CL uses the same logic as implemented in CL 249557.

In

  type T1 struct { t2 }
  type t2 int
  func (t2) M()

T1 has method M because it embeds t2, which has M. Classify the example

  func ExampleT1_M

with T1 instead of ignoring it, as is done currently. There is no other
way to provide an example for such a method, since its original type is
unexported.

Continue to ignore examples on methods from embedded types that are
exported, unless in AllMethods mode. Examples for those methods could be
written on the original type.

The change involves removing a check in classifyExamples. The check
isn't necessary to get the above behavior because
reader.collectEmbeddedMethods and sortedFuncs already generate the
appropriate list of methods.

Updates golang/go#40172

Change-Id: Iaeffc1dcfcfca42dc91db1e2527b10286c06fa84
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/249638
Reviewed-by: Jonathan Amsterdam <jba@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 21, 2020

Change https://golang.org/cl/249557 mentions this issue: go/doc: support examples on methods from embedded unexported types

gopherbot pushed a commit that referenced this issue Aug 21, 2020
In

  type T1 struct { t2 }
  type t2 int
  func (t2) M()

T1 has method M because it embeds t2, which has M. Classify
the example

  func ExampleT1_M

with T1 instead of ignoring it, as is done currently. There is no
other way to provide an example for such a method, since its original
type is unexported.

Continue to ignore examples on methods from embedded types that are
exported, unless in AllMethods mode. Examples for those methods could
be written on the original type.

The change involves removing a check in classifyExamples. The check
isn't necessary to get the above behavior because
reader.collectEmbeddedMethods and sortedFuncs already generate the
appropriate list of methods.

For #40172.

Change-Id: Ibe7d965ecba6426466184e6e6655fc05989e9caf
Reviewed-on: https://go-review.googlesource.com/c/go/+/249557
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot gopherbot added the go.dev label Sep 18, 2020
@julieqiu julieqiu removed the go.dev label Sep 19, 2020
@gopherbot gopherbot added the go.dev label Sep 19, 2020
@julieqiu julieqiu removed the go.dev label Sep 22, 2020
@dmitshur dmitshur added NeedsFix and removed NeedsInvestigation labels Nov 6, 2020
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Nov 7, 2020

I believe this is fixed via CL 249557 for go/doc and CL 249638 for pkg.go.dev. The examples mentioned in the original issue report now show up.

@jba If I missed something and there's more to do here, feel free to reopen.

@dmitshur dmitshur closed this Nov 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants