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/ssa: make a setNumable interface #162

Closed
wants to merge 1 commit into from

Conversation

jirfag
Copy link
Contributor

@jirfag jirfag commented Sep 15, 2019

Workaround crash on go/ssa in go < 1.13.
See golang/go#29612.

Workaround crash on go/ssa in go < 1.13.
See golang/go#29612.
@gopherbot
Copy link
Contributor

This PR (HEAD: 6bf28bf) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/tools/+/195477 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Michael Matloob:

Patch Set 1:

I'm conflicted about this change. I don't know that we want to add code to work around a compiler bug. But if this has been broken since the beginning it's unlikely older versions will be fixed.

I wonder if it's possible to backport the fix to Go 1.12 so that this code works with the last two versions of Go.


Please don’t reply on this GitHub thread. Visit golang.org/cl/195477.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Denis Isaev:

Patch Set 1:

Hi!

I'm conflicted about this change. I don't know that we want to add code to work around a compiler bug.

This fix workarounds crash of programs that include two different versions of go/ssa, e.g. golangci-lint includes x/tools/go/ssa and staticcheck's fork of go/ssa. The bug is in Go < 1.13 compiler, it can be workarounded by not using anonymous interface.

But if this has been broken since the beginning it's unlikely older versions will be fixed.

Before that fix e.g. golangci-lint's latest version users will get runtime error if golangci-lint compiled on go 1.12. After this fix golangci-lint will work ok. So, older versions of go users will be fixed because they will use newer x/tools.

I wonder if it's possible to backport the fix to Go 1.12 so that this code works with the last two versions of Go.

I don't know is it possible.


Please don’t reply on this GitHub thread. Visit golang.org/cl/195477.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Keith Randall:

Patch Set 1:

Patch Set 1:

Hi!

I'm conflicted about this change. I don't know that we want to add code to work around a compiler bug.

This fix workarounds crash of programs that include two different versions of go/ssa, e.g. golangci-lint includes x/tools/go/ssa and staticcheck's fork of go/ssa. The bug is in Go < 1.13 compiler, it can be workarounded by not using anonymous interface.

But if this has been broken since the beginning it's unlikely older versions will be fixed.

Before that fix e.g. golangci-lint's latest version users will get runtime error if golangci-lint compiled on go 1.12. After this fix golangci-lint will work ok. So, older versions of go users will be fixed because they will use newer x/tools.

I wonder if it's possible to backport the fix to Go 1.12 so that this code works with the last two versions of Go.

I don't know is it possible.

We could backport the fix in CL 170157, it should apply cleanly.
I'd rather go with this CL though, as it will fix the problem for all Go versions, and is quite minimal.


Please don’t reply on this GitHub thread. Visit golang.org/cl/195477.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Michael Matloob:

Patch Set 1: Code-Review+2

Patch Set 1:

Patch Set 1:

Hi!

I'm conflicted about this change. I don't know that we want to add code to work around a compiler bug.

This fix workarounds crash of programs that include two different versions of go/ssa, e.g. golangci-lint includes x/tools/go/ssa and staticcheck's fork of go/ssa. The bug is in Go < 1.13 compiler, it can be workarounded by not using anonymous interface.

But if this has been broken since the beginning it's unlikely older versions will be fixed.

Before that fix e.g. golangci-lint's latest version users will get runtime error if golangci-lint compiled on go 1.12. After this fix golangci-lint will work ok. So, older versions of go users will be fixed because they will use newer x/tools.

I wonder if it's possible to backport the fix to Go 1.12 so that this code works with the last two versions of Go.

I don't know is it possible.

We could backport the fix in CL 170157, it should apply cleanly.
I'd rather go with this CL though, as it will fix the problem for all Go versions, and is quite minimal.

Fair enough.


Please don’t reply on this GitHub thread. Visit golang.org/cl/195477.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Michael Matloob:

Patch Set 1: Run-TryBot+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/195477.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=b89e895a


Please don’t reply on this GitHub thread. Visit golang.org/cl/195477.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1:

Build is still in progress...
This change failed on freebsd-amd64-12_0:
See https://storage.googleapis.com/go-build-log/aa680c0c/freebsd-amd64-12_0_0bba1559.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.


Please don’t reply on this GitHub thread. Visit golang.org/cl/195477.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1: TryBot-Result-1

7 of 10 TryBots failed:
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/aa680c0c/freebsd-amd64-12_0_0bba1559.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/aa680c0c/linux-amd64_1fed7c43.log
Failed on linux-386: https://storage.googleapis.com/go-build-log/aa680c0c/linux-386_3febd9d3.log
Failed on openbsd-amd64-64: https://storage.googleapis.com/go-build-log/aa680c0c/openbsd-amd64-64_f13bfd10.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/aa680c0c/windows-386-2008_77aef14a.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/aa680c0c/windows-amd64-2016_cf86c454.log
Failed on linux-amd64-race: https://storage.googleapis.com/go-build-log/aa680c0c/linux-amd64-race_1399f21a.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.


Please don’t reply on this GitHub thread. Visit golang.org/cl/195477.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Denis Isaev:

Patch Set 1:

Patch Set 1: TryBot-Result-1

7 of 10 TryBots failed:
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/aa680c0c/freebsd-amd64-12_0_0bba1559.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/aa680c0c/linux-amd64_1fed7c43.log
Failed on linux-386: https://storage.googleapis.com/go-build-log/aa680c0c/linux-386_3febd9d3.log
Failed on openbsd-amd64-64: https://storage.googleapis.com/go-build-log/aa680c0c/openbsd-amd64-64_f13bfd10.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/aa680c0c/windows-386-2008_77aef14a.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/aa680c0c/windows-amd64-2016_cf86c454.log
Failed on linux-amd64-race: https://storage.googleapis.com/go-build-log/aa680c0c/linux-amd64-race_1399f21a.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

can we merge it?


Please don’t reply on this GitHub thread. Visit golang.org/cl/195477.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Keith Randall:

Patch Set 2: Patch Set 1 was rebased


Please don’t reply on this GitHub thread. Visit golang.org/cl/195477.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Keith Randall:

Patch Set 2: Run-TryBot+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/195477.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 2:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=54535187


Please don’t reply on this GitHub thread. Visit golang.org/cl/195477.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 2: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/195477.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request Oct 1, 2019
Workaround crash on go/ssa in go < 1.13.
See golang/go#29612.

Change-Id: I32687f6ee0baaf223248d5c1631663c73cbbfc65
GitHub-Last-Rev: 6bf28bf
GitHub-Pull-Request: #162
Reviewed-on: https://go-review.googlesource.com/c/tools/+/195477
Run-TryBot: Keith Randall <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Michael Matloob <[email protected]>
@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/195477 has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants