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: add support for type parameters #48264

Closed
findleyr opened this issue Sep 8, 2021 · 18 comments
Closed

x/pkgsite: add support for type parameters #48264

findleyr opened this issue Sep 8, 2021 · 18 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. pkgsite release-blocker

Comments

@findleyr
Copy link
Contributor

findleyr commented Sep 8, 2021

As the proposal for go/ast has stabilized (#47781), I think it is time to start adding support for type parameters in pkgsite.

There is one major obstacle for this, which is that App Engine will not support 1.18 until some time after the 1.18 release, and we want to support type parameters well before then.

My thoughts on how to do this:

  • Write a tool to maintain copies of stdlib packages in pkgsite. I made a working prototype of this in https://golang.org/cl/322411
  • Copy the following packages to pkgsite: go/ast, go/build/constraint, go/format, go/internal/typeparams, go/parser, go/token, go/printer, go/scanner.
  • Wire these in to pkgsite, which already uses a modified copy of go/doc.

I tested that this was possible in https://golang.org/cl/321949.

Once this is done, we should be able to sync these copied packages to tip using go generate. Out of the box this should resolve parsing errors for generic code, and we can proceed with adding any additional features that make it easier to browse code with type parameters in pkgsite. After App Engine supports 1.18, we can delete these copies.

I can do this, but will hold of for a week or two to see if anyone has concerns or better ideas.

CC @julieqiu @jba @jamalc

@findleyr findleyr self-assigned this Sep 8, 2021
@findleyr findleyr added this to the Go1.18 milestone Sep 8, 2021
@findleyr findleyr added release-blocker NeedsFix The path to resolution is known, but the work has not been done. labels Sep 8, 2021
@dmitshur
Copy link
Contributor

dmitshur commented Sep 8, 2021

I understand the scope of this issue is x/pkgsite, and I don't mean to change that.

I'll point out that a similar issue may apply to x/website, though golang.org serves documentation under a significantly reduced set of use cases as of #44356. It similarly has a directory with ported stdlib packages (https://go.googlesource.com/website/+/refs/heads/master/internal/backport/).

@jba
Copy link
Contributor

jba commented Sep 9, 2021

I'm in favor. There is an alternative: move pkgsite to Cloud Run. That would probably be more work, and certainly more risk, but I just wanted to put it out there.

One point you don't mention: none of those packages would be able to use 1.18 features (including generics) for a while. And we won't know that they do until we copy them and recompile pkgsite. On the other hand, I don't imagine they'll change much once generics are done.

@findleyr
Copy link
Contributor Author

findleyr commented Sep 9, 2021

One point you don't mention: none of those packages would be able to use 1.18 features

Right, that's a good point. In https://golang.org/cl/321949 I had to add a stub io/fs for this reason.

IIRC @griesemer and I had discussed and thought this was OK for these packages in particular. We also have a history of maintaining ports like x/tools/go/internal/gcimporter, but that's more work.

@earthboundkid
Copy link
Contributor

FWIW, as a user, I'm writing some generic packages against gotip, and I find it really annoying that I can't read HTML formatted docs yet. If this can ship before 1.18, it would be great for me.

@findleyr
Copy link
Contributor Author

@carlmjohnson this will definitely ship before 1.18.

Since no objections and #47781 has been accepted, I will start soon.

@toothrot
Copy link
Contributor

Re-milestoning, as we will not be blocking the Go 1.18 release on this issue.

@toothrot toothrot modified the milestones: Go1.18, Unreleased Sep 22, 2021
@jamalc jamalc modified the milestones: Unreleased, pkgsite/unplanned Sep 22, 2021
@sfllaw
Copy link
Contributor

sfllaw commented Sep 24, 2021

I'm in favor. There is an alternative: move pkgsite to Cloud Run. That would probably be more work, and certainly more risk, but I just wanted to put it out there.

As someone who runs an internal pkgsite, I have been building the frontend Docker image and have had good experiences. We don’t run on Google Cloud and there’s an argument that pkgsite should be more agnostic when it comes to hosting?

@findleyr
Copy link
Contributor Author

findleyr commented Nov 9, 2021

It looks like @jba will be able to move pkgsite to Cloud Run, which simplifies things significantly. Reassigning.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/382974 mentions this issue: pkgsite: prepare for go 1.18

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/382976 mentions this issue: internal/fetch: add generics test

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/382975 mentions this issue: internal/godoc/internal/doc: update go go1.18beta1

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/382977 mentions this issue: internal/godoc: make work with generics

gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 4, 2022
For golang/go#48264

Change-Id: I55e512051450fb4bbc58fe00872c5d7077988ccb
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/382974
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
TryBot-Result: kokoro <noreply+kokoro@google.com>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 4, 2022
Incorporate the changes in go/doc made for generics into our fork of
go/doc.

For golang/go#48264

Change-Id: Ic31d0d58ffbe731b5d425506564ecda3774ae840
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/382975
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 4, 2022
For golang/go#48264

Change-Id: I94b1bae5b2a59753492148edf2e1f2931235802f
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/382976
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
TryBot-Result: kokoro <noreply+kokoro@google.com>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 4, 2022
We need to register a new AST node to support
encoding.

Also add a test.

For golang/go#48264

Change-Id: Ib9a0205ff4daeb55ee3447945fca513d2e4cb9c8
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/382977
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
TryBot-Result: kokoro <noreply+kokoro@google.com>
@jba
Copy link
Contributor

jba commented Feb 16, 2022

This is live.

@jba jba closed this as completed Feb 16, 2022
@gmlewis
Copy link
Contributor

gmlewis commented Feb 16, 2022

@jba - how do we get previously-scanned sites to be updated?
For example, https://pkg.go.dev/github.com/gmlewis/advent-of-code-2021 is still missing all the packages that utilize generics.

@hyangah
Copy link
Contributor

hyangah commented Feb 16, 2022

@gmlewis Is it possible to push a new version? I don't think old versions are re-indexed (pkgsites db grew so big), but new ones will be indexed. Since go1.18 generics spec went through a couple of last minute changes recently, I think it's a good idea to push a fresh new one.

@gmlewis
Copy link
Contributor

gmlewis commented Feb 16, 2022

@gmlewis Is it possible to push a new version? I don't think old versions are re-indexed (pkgsites db grew so big), but new ones will be indexed. Since go1.18 generics spec went through a couple of last minute changes recently, I think it's a good idea to push a fresh new one.

@hyangah - that worked! Thank you very much!

@anacrolix
Copy link
Contributor

Sorry for the confusion, but what is live? Should https://pkg.go.dev/ be working with generic code? Is there a sample URL?

@hyangah
Copy link
Contributor

hyangah commented Feb 16, 2022

@anacrolix https://pkg.go.dev/golang.org/x/exp@v0.0.0-20220212023102-3e31098684e2/slices

@rsc rsc unassigned jba Jun 23, 2022
@golang golang locked and limited conversation to collaborators Jun 23, 2023
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. pkgsite release-blocker
Projects
None yet
Development

No branches or pull requests