feat(Optimize Go imports): run goimports on Go target code #4777
Conversation
| @@ -87,6 +87,22 @@ jobs: | |||
| with: | |||
| java-version: 18 | |||
| distribution: corretto | |||
| - name: Set up newest supported Go | |||
| if: matrix.target-language-version == 'newest' | |||
There was a problem hiding this comment.
Just FYI, we aren't currently ever populating matrix.target-language-version, because running the entire suite twice for the sake of a small percentage of tests wasn't frugal. See #1983 for more background, especially my latest comment on how I think we should solve this problem.
There was a problem hiding this comment.
This might be the reason the CI is still failing for you in fact.
There was a problem hiding this comment.
That's why I wasn't seeing the Go 1.20 execution, I thought it was skipped due to Go 1.11 failures! I have removed the check for newest and Go 1.20. Added Go 1.15 (min Go version supported by Aws Sdk) as the version to test against instead of older 1.11.
| /// </summary> | ||
| public virtual void OnPostCompile() { } | ||
| public abstract bool OnPostCompile(string dafnyProgramName, string targetFilename, TextWriter outputWriter); |
There was a problem hiding this comment.
I think it makes more sense to keep this as an empty virtual method, so implementing classes don't have to do anything if they don't need this hook.
There was a problem hiding this comment.
Also targetFilename should probably be targetDirectory, since you're passing targetPaths.SourceDirectory for it.
There was a problem hiding this comment.
I needed a return type to convey if the method OnPostCompile has succeeded or not. But with a return type on a virtual method, I’m forced to return true on the base virtual method. It might be a bit better to have the person implementing OnPostCompile reason about what to do OnPostCompile than we biasing them towards doing nothing.
Yup, I prototyped with a file first and hence the name. Fixed.
| @@ -205,7 +205,8 @@ To set up Dafny to compile to Go: | |||
| * Install Go from [https://golang.org/dl](https://golang.org/dl) or | |||
| * Linux: `sudo apt install golang` | |||
| * Mac: `brew install golang` | |||
| * Make sure `go` is on your path | |||
| * Install `goimports` from [https://pkg.go.dev/golang.org/x/tools/cmd/goimports](https://pkg.go.dev/golang.org/x/tools/cmd/goimports) | |||
There was a problem hiding this comment.
An explicit command to execute as well would be slightly friendlier
There was a problem hiding this comment.
Added go install golang.org/x/tools/cmd/goimports@latest . This installs the latest goimports based on the latest go installation from previous command.
| @@ -4,47 +4,47 @@ | |||
| package _System | |||
|
|
|||
| import ( | |||
| _dafny "dafny" | |||
| os "os" | |||
| _dafny "dafny" | |||
There was a problem hiding this comment.
I take it these changes are because goimports applies auto-formatting as well?
No issues with that and in fact that's kind of a nice side-effect of this change!
Better compatibility across more Go versions
### Description Should fix nightly build break caused by #4777 <small>By submitting this pull request, I confirm that my contribution is made under the terms of the [MIT license](https://github.com/dafny-lang/dafny/blob/master/LICENSE.txt).</small>
Description
This PR adds support to optimize imports in the generated Go target code. It runs the
goimportstool to do the optimization.How has this been tested?
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.