cmd/go/internal/generate: stop premature variable substitution in commands #31527

QuasarSE · 2019-04-17T20:13:30Z

go:generate commands passed no arguments are currently subject
to premature variable substitution due to mistakenly assuming append
guarantees a copy. The change fixes this by forcing a slice copy at
each invocation of a command.

The previous code assumed that append would always generate a
copy of its inputs. However, append wouldn't create a copy if there was
no need to increase capacity and it would just return the original
input slice. This resulted in premature variable substitutions in
the "master word list" of generate commands, thus yielding incorrect
results across multiple invocations of the same command when the
body contained substitutions e.g. environment variables, moreover
these can change during the lifetime of go:generate processing a
file.

Note that this behavior would not manifest itself if any arguments were
passed to the command, because append would make a copy of the slice
as it needed to increase its capacity. The "hacky" work-around was to
always pass at least one argument to any command, even if the
command ignores it. e.g.,
//go:generate MyNoArgsCmd ' '

This CL fixes that issue and removes the need for the hack mentioned
above.

Fixes #31608

The current code assumes (and mentions) that append() will always generate a copy of inputs. The problem is that append() doesn't if there was no need to increase capacity, instead just returning a reference to the original input array. This results in premature variable substitutions in generate commands rather than in copies of the words in that command, yielding incorrect results across multiple invocations of a command when the body contains substitutions (e.g. environment variables) which can change during go:generate processing of the file. Note that this behavior does not manifest itself if any arguments are passed to the command, due to append() making a copy since it required a capacity boost. The work-around is, unintuitively, to always pass at least one argument to any command, even if the command ignores it. e.g., //go:generate MyNoArgsCmd '' works around the problem. The proposed code change corrects the substitution problems, removing the need for this workaround "trick".

googlebot · 2019-04-17T20:13:33Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

QuasarSE · 2019-04-17T20:18:51Z

CLA now signed.

googlebot · 2019-04-17T20:18:54Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

gopherbot · 2019-04-17T20:20:46Z

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

Please visit https://go-review.googlesource.com/c/go/+/172580 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 · 2019-04-17T20:23:08Z

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

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

gopherbot · 2019-04-17T22:31:50Z

Message from Ian Lance Taylor:

Patch Set 1:

(6 comments)

Can you add a test?

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

gopherbot · 2019-04-17T23:48:19Z

Message from Jay Conrod:

Patch Set 1:

(2 comments)

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

gopherbot · 2019-04-22T05:20:04Z

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

Please visit https://go-review.googlesource.com/c/go/+/172580 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 · 2019-04-22T09:15:04Z

Message from Emmanuel Odeke:

Patch Set 2: Run-TryBot+1

(2 comments)

Hello Shawn, thank you for this change and welcome to the Go project!

Jay and Ian provided some feedback on your CL; please open the corresponding
issue as suggested and then add it in your commit message. I have posted up
some suggestions for your commit message, please take a look.

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

gopherbot · 2019-04-22T09:16:10Z

Message from Gobot Gobot:

Patch Set 2:

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

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

gopherbot · 2019-04-22T09:24:41Z

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/172580.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-04-22T14:15:48Z

This PR (HEAD: 4b61931) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/172580 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

QuasarSE · 2019-04-22T14:18:19Z

Review suggestions incorporated. Tests added -- will fail with existing code, pass with new.

gopherbot · 2019-04-22T14:22:57Z

Message from Shawn Elliott:

Patch Set 8:

Thanks for the review suggestions... all incorporated, not sure if I need to do anything past "reply" to move it ahead.

Shawn.

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

gopherbot · 2019-04-22T14:24:37Z

Message from Shawn Elliott:

Patch Set 8:

Patch Set 1:

(6 comments)

Can you add a test?
Test series added. thx.

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

gopherbot · 2019-04-22T14:45:23Z

This PR (HEAD: 796d343) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/172580 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 · 2019-04-22T14:48:10Z

Message from Shawn Elliott:

Patch Set 9:

Patch Set 1:

(2 comments)

Sorry Jay, I read the point on the comment backwards. Corrected in source to a much shorter comment text mentioning why the code is doing the copy.

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

gopherbot · 2019-04-22T18:22:41Z

Message from Jay Conrod:

Patch Set 9: Run-TryBot+1

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

gopherbot · 2019-04-22T18:23:34Z

Message from Gobot Gobot:

Patch Set 9:

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

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

gopherbot · 2019-04-22T18:35:50Z

Message from Gobot Gobot:

Patch Set 9: TryBot-Result+1

TryBots are happy.

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

gopherbot · 2019-04-22T18:35:51Z

Message from Jay Conrod:

Patch Set 9: Code-Review+2

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

…mands go:generate commands passed no arguments are currently subject to premature variable substitution due to mistakenly assuming append guarantees a copy. The change fixes this by forcing a slice copy at each invocation of a command. The previous code assumed that append would always generate a copy of its inputs. However, append wouldn't create a copy if there was no need to increase capacity and it would just return the original input slice. This resulted in premature variable substitutions in the "master word list" of generate commands, thus yielding incorrect results across multiple invocations of the same command when the body contained substitutions e.g. environment variables, moreover these can change during the lifetime of go:generate processing a file. Note that this behavior would not manifest itself if any arguments were passed to the command, because append would make a copy of the slice as it needed to increase its capacity. The "hacky" work-around was to always pass at least one argument to any command, even if the command ignores it. e.g., //go:generate MyNoArgsCmd ' ' This CL fixes that issue and removes the need for the hack mentioned above. Fixes #31608 Change-Id: I782ac2234bd7035a37f61c101ee4aee38ed8d29f GitHub-Last-Rev: 796d343 GitHub-Pull-Request: #31527 Reviewed-on: https://go-review.googlesource.com/c/go/+/172580 Run-TryBot: Jay Conrod <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Jay Conrod <[email protected]>

gopherbot · 2019-04-22T18:36:52Z

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

googlebot added the cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. label Apr 17, 2019

googlebot added cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. and removed cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. labels Apr 17, 2019

Updated with additional Tests

c2f0263

QuasarSE changed the title ~~generate: stop premature variable substitution in commands~~ cmd/generate: stop premature variable substitution in commands Apr 22, 2019

QuasarSE changed the title ~~cmd/generate: stop premature variable substitution in commands~~ cmd/go/internal/generate: stop premature variable substitution in commands Apr 22, 2019

Incorporated review comments

4b61931

shortened comment as per review suggestion

796d343

gopherbot closed this Apr 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/go/internal/generate: stop premature variable substitution in commands #31527

cmd/go/internal/generate: stop premature variable substitution in commands #31527

QuasarSE commented Apr 17, 2019 •

edited

Loading

googlebot commented Apr 17, 2019

QuasarSE commented Apr 17, 2019

googlebot commented Apr 17, 2019

gopherbot commented Apr 17, 2019

gopherbot commented Apr 17, 2019

gopherbot commented Apr 17, 2019

gopherbot commented Apr 17, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

QuasarSE commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

cmd/go/internal/generate: stop premature variable substitution in commands #31527

cmd/go/internal/generate: stop premature variable substitution in commands #31527

Conversation

QuasarSE commented Apr 17, 2019 • edited Loading

googlebot commented Apr 17, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

QuasarSE commented Apr 17, 2019

googlebot commented Apr 17, 2019

gopherbot commented Apr 17, 2019

gopherbot commented Apr 17, 2019

gopherbot commented Apr 17, 2019

gopherbot commented Apr 17, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

QuasarSE commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

gopherbot commented Apr 22, 2019

QuasarSE commented Apr 17, 2019 •

edited

Loading