-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
all: merge dev.typeparams to master during Go 1.17 #43931
Comments
Just to clarify, this was not intended to deride #43810. Persistent memory was brought up as a counter-point in the Google compiler/runtime team meeting, which I understood at the time to be a hypothetical proposal. I vaguely remembered the topic of Go language support for persistent memory being discussed in the past over email, but I was not aware it had yet advanced to a proposal. I'm not presently familiar enough with persistent memory to know whether #43810 is a good idea, and I don't mean to imply either way about it here. But I do think its current thumbs up/down ratio demonstrates a different level of broad community support than #43651, which was the point I was arguing for why I think it's justified as in the interest of the broad Go community to allow development for #43651 to continue on master behind a feature flag, when we probably wouldn't extend the same offer to #43810. |
I don't think we can land code on master before a proposal is accepted. That goes against the spirit of the proposal process and the rules we ask others to follow for proposed changes. I think the right way to view this proposal is as simply to start working on master once #43651 is accepted, changing our previous plan to stay on dev.typeparams until Go 1.18. My bigger concern is how to properly firewall off the generics work from the Go 1.17 work so that changes being made to help set up for generics don't cause undue instability in the Go 1.17 release. How many code changes do you contemplate (for example in the back ends) that would not be gated by the flag? |
English isn't my first language, but that line sounds circular to me. No proposal can be against the spirit of the proposal process, it is a proposal. |
I think Russ is being taken slightly out of context there. I understood his comment to say: we've historically not merged code before a proposal is accepted, so we should not make an exception here. It might set a bad precedent, which is something I can understand. That said, I think this proposal is special, given how much it's refactoring and improving the compiler and typechecker. I assume we want those merged in the future, even if generics end up being rejected. So I understand Matthew's point about the large amount of extra tedious work involved, which could be avoided. |
Landing code on master implementing a proposal before it is accepted is at odds with what the proposal process exists for. There is no implication that the filing of this proposal goes against that. I can perhaps see how the confusion arose, so please consider this to be a clarifying point, as I was in attendance when @rsc’s response was written. |
That's not what he's saying. He's saying that the point of the proposal process is to have a standardized way to get new features into the language. If a proposal, in this case #43651, is given special treatment and allowed to circumvent part of that process, then it is unfair to other proposals and sets a bad precedent for later ones because people will want to know why their popular but still unaccepted proposal can't get that treatment, too. |
Few proposals have the scope of the generics work. I think it’s ok to make exceptions, and if people need to understand why exceptions were made it’s easy to explain. |
Not all proposals will have the same scope. I think what is important is for an explanation to be given as to why a certain proposal requires an exemption to the rules. For you to obtain such an exemption, what do you do? You write a proposal. That is what Mdempsky has done. It is up to others, then to figure out if the reasons given are enough to warrant the exemption. |
To my knowledge #43651 still doesn't even have a specific proposal for the changes to the language spec (see #43651 (comment)), which seems like a necessary step for any serious proposal to change the language. But perhaps that step is waiting on detail to be informed by the implementation work. #43651 sketches out a design that requires a lot of refactoring and implementation work. Some of the refactoring work seems outside the scope of the proposal process in the first place (since it has no user-facing effect on the toolchain). On top of that, the user-facing implementation work seems like a fine application for I would much rather we accept the implementation work as a |
As the de facto primary build-cop for the project over the past couple of years, I'm honestly more concerned with baking in regressions on the branch. We already have what is in my opinion a severe problem with regressions being introduced into the tree and dismissed as “flakes”, or accepted as likely bugs but not triaged promptly because they occur with low frequency. With regressions introduced on a separate branch, it is even easier to (intentionally or accidentally) bury regressions that happen to manifest as intermittent failures. I would rather we deal with those regressions in the main branch when they are introduced, rather than accumulating a backlog of regressions in the branch and spending a month or more dealing with an unstable tree after the branch is merged back in. |
I really can't comment on the “spirit of the proposal process” line, but isn't the goal of the proposal process making sure that the changes are well designed? Features often get designed one way only for that original design to be improved in the process of implementing it and discovering its inconsistencies and omissions. The Regarding the possible instability in Go 1.17. Type parameters are possibly the most anticipated feature since Go has been publicly released. I think it's reasonable to assume that many people will build Go from the main branch once the type parameters work is merged. They will want to try the new features, play with them, and report bugs. The bugs some of which will show flaws in the design. Especially if there is a call for help to the community, for example through the Go Blog. |
Would it be possible to accept #43651 in principle without requiring the proposal to be finalised to the very last detail? ISTM that the basic outline of the proposal is solid, and there's considerable room for movement around remaining semantic details while still keeping the overall model intact. Once #43651 is accepted in principle, further proposals could be introduced to nail down specific semantics for some of the more awkward parts, but ISTM that those are unlikely to invalidate the preparatory work currently being undertaken in the dev.typeparams branch. That would allow work to start on the master branch even while details are being finalised. |
Maybe? But that's in tension with Russ's comment:
“[T]he rules we ask others to follow for proposed changes” are, in part, answering the questions in the Go 2 language change template, including:
I believe that the current design document satisfies only the first of those five points. In my opinion, “a class teaching Go” would need to include answers to basic questions like “what is a type?” and “what is an interface?”, which are covered in the existing spec but notably missing from the current proposal — despite the concerns I raised last summer and re-raised in #43651 (comment) about those concepts. |
This has gotten a bit heated, and even though it has cooled down, I'm going to lock it until next week. |
Last week I wrote:
Continuing to view this proposal as "merge dev.typeparams to master during Go 1.17 dev cycle", this seems like a reasonable thing to do. I also asked:
We should make sure that we are careful to keep risky changes gated by the flag. We should also probably disable the flag in the Go 1.17 release itself, so that we don't end up with an ecosystem of packages that have to be built with Are there other precautions we should take to make sure that merging this code to master does not cause undue risk in Go 1.17? |
I should add that one of the most compelling reasons for making this change is that it creates backpressure on non-generics work to make sure that the work is compatible with what's going on in dev.typeparams. Without the merge to master, all the merges from master end up creating work for the team working on generics, with no feedback to the master-branch work that changes are being made incompatibly. |
I'm just as eager as anyone for #43651 to be accepted, however I agree with @rsc and @bcmills that we should follow the procedure for language changes. I know I did when I made my failed proposals >;-> So the question is then: how can we speed up the acceptance of #43651? Could the Go team shift a few people, or perhaps ask Google if they are willing to hire some more help? Can we, outside of the Go team, contribute somehow, perhaps with the informal description if the feature? Once #43651 is accepted, this proposal could also be accepted to speed up implementation then, hidden behind a flag or GOEXPERIMENT, whichever is most effective and convenient. |
I see code changes falling into three camps:
I think the camp 1 changes are largely done already on dev.typeparams; I think we've already run that option about as far as possible. I think most remaining work will be split between camps 2 and 3. I don't currently have any solid predictions of what the ratio might be, but I expect there will be significant work still falling into each camp and diffuse through a lot of parts of the compiler. For example, I think we'll want DWARF support for generic code/types. This will probably require both refactoring some of the current DWARF code (camp 3), and then adding support for generating the new DWARF DIEs (camp 2). The refactored code will continue benefiting from existing tests and development practices (e.g., testing CLs with I'd generally expect changes to be more in camp 2 closer to the frontend (e.g., transforming generic code into IR), and more in camp 3 closer to the backend (e.g., plumbing generics metadata through as needed for debugging/runtime).
Ack.
I think disabling generics support in the Go 1.17 release builds would be fine. More broadly, I think it would be nice to better delineate between compiler flags/extensions that are stable and supported for direct use by end users, and unstable flags that are only there for use by compiler developers and/or the standard library. But that's a separate issue.
I think the best precaution is merging as early as possible / reasonable. The master branch is the most tested/used development branch. The earlier dev.typeparams is merged, the more time+testing we'll have to make sure changes haven't negatively impacted the non-generics code paths.
Also, merges are already difficult enough to create and review, and there's no good workflow for breaking tricky merge conflicts into smaller CLs like you would with normal development, except to merge more frequently. Each merge is all or nothing. Reviewing merge conflicts are also error-prone / tedious: Gerrit only presents the specific conflicts and how they were resolved. It doesn't provide an easy way to see the changes in their full, original contexts, so reviewers have to go out of their way to track down this information if needed. Finally, mistakes introduced by bad merge conflict resolutions will be harder to diagnose later. E.g., if Overall, I think merging dev.typeparams to master for Go 1.17 and doing development on master does increase risk somewhat by simple nature of more changes -> more opportunity for mistakes. But I think that risk is already offset by the increased testing exposure working on master and also the additional engineer time freed up by not having to worry about merges. And I think it will also significantly reduce the risks for enabling generics in Go 1.18 (assuming #43651 is accepted). |
When the master 'hijack' is taking place I'd like to have pipelined improvements from all relevant branches in the EXPERIMENT... so that we could see their effects on existing codebases in one Go2 pre-Release 'testing suite'. |
Based on the discussion above, this proposal seems like a likely accept. |
O M G, I DID NOT THINK THIS WILL HAPPEN IN MY LIFE TIME. |
How can I use this to play with generics? Tried using the $ gotip run -gcflags=all=-G=2 main.go
go build internal/unsafeheader: open /tmp/go-build1866398939/b005/_pkg_.a: no such file or directory
go build internal/race: open /tmp/go-build1866398939/b021/_pkg_.a: no such file or directory
go build internal/abi: open /tmp/go-build1866398939/b008/_pkg_.a: no such file or directory
go build math/bits: open /tmp/go-build1866398939/b017/_pkg_.a: no such file or directory
go build runtime/internal/sys: open /tmp/go-build1866398939/b013/_pkg_.a: no such file or directory
go build unicode/utf8: open /tmp/go-build1866398939/b019/_pkg_.a: no such file or directory
os.Stat of archive file failed: stat /tmp/go-build1866398939/b011/_pkg_.a: no such file or directory |
@elichai Currently you still need to use Beware that the generics-specific support is still very early. Non-generic Go code is expected to fully work, so please file issues if you find anything that works without |
Change https://golang.org/cl/295029 mentions this issue: |
@mdempsky Hi, do you understand why -G=3 doesn't work with go module? |
What's exactly command do you run? Maybe try |
@cuonglm Ive tried with |
@dzpt then you may want to try dev.typeparams instead, not all generic features are available on tip. |
@cuonglm it works without go.mod, so i don't think generic code causes the error.
generics works on main.go, but got error on generics/xys.go |
What version specify in your go.mod? |
@cuonglm 1.17
|
Then I dont think it will work, as generic syntax is only available in go1.18 |
@cuonglm then you might not read this issue https://github.com/mattn/go-generics-example |
@dzpt Can you share your code instead? (Sorry for confusion in previous comment, what I mean is not all generic syntax is available in go1.17) Also, can be helpful if you can post |
@cuonglm the code is here https://github.com/dzpt/test-go-generics it works fine with same generics code in main.go, but failed for |
@dzpt I got this:
Like I said, not all generic features available in go1.17 . I don't think go modules is relevant here. My gotip version:
|
@dzpt Sorry, generics are not supported in Go 1.17. Full stop. If you want to help test generics for Go 1.18, you can use the dev.typeparams branch and file new issues at golang.org/issue/new. This issue is closed. Thanks. |
@dzpt please upgrade your module file to require 1.18, if you use above gotip version, then it will work.
|
@dzpt Also note that your above one technically is not Your above comment indicate that you are using dev.typeparams, so the code structure, and support generic code are way difference compare to master. That why when your module require go1.17, your gotip can't parse generic syntax. |
@cuonglm thank you, it works now after changing go mod 1.17 to 1.18 |
What
Proposal #43651 is to add generics to the Go language. The proposal currently has 1223 likes and only 89 dislikes. Demand for generics was a top requested feature in the 2016, 2017, 2018, and 2019 Go developer survey results. There's a working go2go Playground for trying out the proposal. The type parameters proposal was the subject of a keynote talk at GopherCon 2020. This is a widely demanded feature from the Go community; it's not a niche feature like persistent memory or field tracking.
This proposal then is about allowing prototype compiler development of that feature to happen on the master branch. The new language functionality would be kept behind a compiler flag until both (1) #43651 is accepted and (2) the functionality is ready for end users.
Of course, if #43651 is accepted before this proposal, then this one becomes moot. Alternatively, should #43651 be rejected after this one is accepted, then the implementation support code should be backed out of master.
Rationale
We've already begun development for #43651 on the dev.typeparams branch, but keeping it in sync with master and dev.regabi has been very tedious. Within the last 2 months of development, we've had 20+ merges between branches. We had a failed merge that required resetting the development branches (#43147). We've had compiler engineers attempt and give up merges, because the conflicts were too difficult to resolve without more familiarity/time.
This tedium will likely decrease somewhat after Go 1.16 is released and dev.regabi merges back into master, but then there will also be changes happening elsewhere in the repo to keep up with (e.g., #42637 caused a lot of trybot flakes on both dev.regabi and dev.typeparams). Also, support for generics will continue requiring more pervasive changes that involve touching the backend. There are also other accepted Go language changes slated for Go 1.17 (e.g., #395, #19367, #38248, #40481) that will overlap in frontend areas.
It would ease cmd/compile development significantly if the compiler functionality in dev.typeparams could be merged into master and developed behind a feature flag. Feature flags are how we developed binary export (ae2f54a), SSA (c0740fe), package syntax's new parser (2ff4639), indexed export (ca2f85f), new escape analysis (97c4ad4), etc. (I'm sure there are others; these are just ones I'm personally familiar with.)
Additionally, dev.typeparams introduces a new go/types-based typechecker "types2". Even if we reject #43651, I think we want to adopt this new typechecker to replace the compiler's legacy typechecker ("typecheck"). And because generic support in types2 is already conditional behind a flag, removing generics support if #43651 is rejected would be no different than when we removed the old code any of the above features replaced.
Flag
As for how to spell the compiler flag, dev.typeparams currently uses
-G
, but other spellings would be fine. The particular values assigned to it though are somewhat arbitrary, driven by development needs that no longer apply, so they should be revisited. I suggest-G=0
means use typecheck;-G=1
means use types2 w/o generics support; and-G=2
means use types2 w/ generics support. (I'm hopeful-G=1
can be the default for Go 1.17; then we can drop support for-G=0
for Go 1.18, which should make it easier to enable-G=2
by default. However, I'm not proposing either of these default changes at this time.)It's been brought up that users might then use it and things could break. We currently provide users with compiler flags like
-B
(disable bounds checking),-wb=false
(disable write barriers), and-d=disablenil
(disable nil checking), so users have ample opportunities for self-harm today already.But moreover, I think we want users trying out features before they're fully ready. It's been helpful having users report regressions on dev.regabi (#43479, #43480, #43701, #43818), and at the end of release cycles we're always begging for users to try the betas and release candidates.
If really desired, we could put it behind
GOEXPERIMENT=typeparams
with other features like field tracking and static lock ranking, which never went through the proposal process. But then I'd ask again for consideration of #42681.The text was updated successfully, but these errors were encountered: