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

feat(Optimize Go imports): run goimports on Go target code #4777

Merged
merged 35 commits into from
Jan 24, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
9ad39a8
feat(Optimize Go imports): run goimports on Go target code
ShubhamChaturvedi7 Nov 14, 2023
c8b7166
Merge branch 'dafny-lang:master' into master
ShubhamChaturvedi7 Nov 14, 2023
2b03658
fix(docs): Update the Installation.md for Go
ShubhamChaturvedi7 Nov 16, 2023
2ed2b92
Merge branch 'master' into master
ShubhamChaturvedi7 Nov 16, 2023
79b2a41
fix(CI): Go get goimports
ShubhamChaturvedi7 Nov 16, 2023
2c31098
Merge branch 'master' into master
ShubhamChaturvedi7 Nov 16, 2023
375b49b
fix(CI): Go install goimports
ShubhamChaturvedi7 Nov 16, 2023
977f05b
fix(CI): Use setup-go for goimports
ShubhamChaturvedi7 Nov 17, 2023
9821ca0
Merge branch 'master' into master
ShubhamChaturvedi7 Nov 17, 2023
418ca76
fix(CI Tests): Diff formatting fix
ShubhamChaturvedi7 Nov 17, 2023
2f07ff4
Merge branch 'dafny-lang:master' into master
ShubhamChaturvedi7 Nov 28, 2023
5308135
Merge branch 'master' into master
ShubhamChaturvedi7 Nov 29, 2023
1a7edcf
fix(CI): Install goimports for Go 1.11
ShubhamChaturvedi7 Nov 29, 2023
4561a2e
fix(CI): Install goimports for Go 1.20
ShubhamChaturvedi7 Nov 29, 2023
397910f
fix(Review): Comments
ShubhamChaturvedi7 Dec 1, 2023
8ec957f
Merge branch 'master' into master
ShubhamChaturvedi7 Dec 1, 2023
cf2bb36
fix(CI): Use the min compatible version of Go for CI
ShubhamChaturvedi7 Dec 1, 2023
5df8644
fix(CI): Use GO111MODULE=auto
ShubhamChaturvedi7 Dec 1, 2023
c2ade99
fix(CI): Use GO111MODULE=on
ShubhamChaturvedi7 Dec 1, 2023
8092f3c
Merge branch 'master' into master
ShubhamChaturvedi7 Dec 4, 2023
5cd7107
Merge branch 'master' into master
ShubhamChaturvedi7 Dec 6, 2023
5782838
Merge branch 'master' into master
robin-aws Dec 8, 2023
9d24383
fix(CI): Fixes CHECK directive across Go Versions
ShubhamChaturvedi7 Dec 12, 2023
fa31f66
Merge branch 'master' into master
ShubhamChaturvedi7 Dec 12, 2023
1e4b095
Merge branch 'master' into master
ShubhamChaturvedi7 Dec 12, 2023
9b86632
Merge branch 'master' into master
ShubhamChaturvedi7 Dec 13, 2023
1935f0b
Merge branch 'master' into master
ShubhamChaturvedi7 Dec 13, 2023
899cb5d
Merge remote-tracking branch 'aws/master'
ShubhamChaturvedi7 Jan 4, 2024
55eb801
fix(CI): Merge conflict fix
ShubhamChaturvedi7 Jan 4, 2024
2b5aafa
Merge branch 'dafny-lang:master' into master
ShubhamChaturvedi7 Jan 12, 2024
36af5f7
Use ioutil instead of os in Go Std extern code
robin-aws Jan 19, 2024
bac9d4e
Merge branch 'master' into master
robin-aws Jan 19, 2024
0ba2131
Merge branch 'master' into master
robin-aws Jan 19, 2024
15050d8
Typo
robin-aws Jan 19, 2024
e19d9ba
Merge remote-tracking branch 'aws/master'
ShubhamChaturvedi7 Jan 24, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .github/workflows/integration-tests-reusable.yml
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,22 @@ jobs:
with:
java-version: 18
distribution: corretto
- name: Set up newest supported Go
if: matrix.target-language-version == 'newest'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be the reason the CI is still failing for you in fact.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

uses: actions/setup-go@v4
with:
go-version: '1.20'
- name: Set up oldest supported Go
if: matrix.target-language-version != 'newest'
uses: actions/setup-go@v4
with:
go-version: '1.11'
- name: Set up goimports using GOPATH
if: matrix.target-language-version != 'newest'
run: go get golang.org/x/tools/cmd/goimports
- name: Set up goimports using GO MODULE
if: matrix.target-language-version == 'newest'
run: go install golang.org/x/tools/cmd/goimports@latest
- name: Set up Python
uses: actions/setup-python@v4
with:
Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/runtime-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ jobs:
with:
java-version: 18
distribution: corretto
- name: Set up Go
uses: actions/setup-go@v4
with:
go-version: '1.20'
- name: Set up goimports
run: go install golang.org/x/tools/cmd/goimports@latest
- name: Build Dafny
run: dotnet build Source/Dafny.sln
- name: Get Z3
Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/standard-libraries.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ jobs:
with:
java-version: 18
distribution: corretto
- name: Set up Go
uses: actions/setup-go@v4
with:
go-version: '1.20'
- name: Set up goimports
run: go install golang.org/x/tools/cmd/goimports@latest
- name: Build Dafny
run: dotnet build Source/Dafny.sln
- name: Get Z3
Expand Down
4 changes: 2 additions & 2 deletions Source/DafnyCore/Compilers/ExecutableBackend.cs
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,9 @@ SinglePassCompiler Compiler {
}
}

public override void OnPostCompile() {
base.OnPostCompile();
public override bool OnPostCompile(string dafnyProgramName, string targetFilename, TextWriter outputWriter) {
Compiler.Coverage.WriteLegendFile();
return true;
}

protected abstract SinglePassCompiler CreateCompiler();
Expand Down
15 changes: 15 additions & 0 deletions Source/DafnyCore/Compilers/GoLang/GoBackend.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,21 @@ protected override SinglePassCompiler CreateCompiler() {
return new GoCompiler(Options, Reporter);
}

public override bool OnPostCompile(string dafnyProgramName, string targetFilename, TextWriter outputWriter) {
return base.OnPostCompile(dafnyProgramName, targetFilename, outputWriter) && OptimizeImports(targetFilename, outputWriter);
}

public bool OptimizeImports(string targetFilename, TextWriter outputWriter) {
var goArgs = new List<string> {
"-w",
targetFilename
};

var psi = PrepareProcessStartInfo("goimports", goArgs);

return 0 == RunProcess(psi, outputWriter, outputWriter);
}

public override bool CompileTargetProgram(string dafnyProgramName, string targetProgramText, string/*?*/ callToMain,
string/*?*/ targetFilename, ReadOnlyCollection<string> otherFileNames,
bool runAfterCompile, TextWriter outputWriter, out object compilationResult) {
Expand Down
3 changes: 2 additions & 1 deletion Source/DafnyCore/Compilers/Library/LibraryBackend.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,9 @@ protected override SinglePassCompiler CreateCompiler() {
return null;
}

public override void OnPostCompile() {
public override bool OnPostCompile(string dafnyProgramName, string targetFilename, TextWriter outputWriter) {
// Not calling base.OnPostCompile() since it references `compiler`
return true;
}

public override string PublicIdProtect(string name) {
Expand Down
4 changes: 2 additions & 2 deletions Source/DafnyCore/Plugins/IExecutableBackend.cs
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,9 @@ public virtual void OnPreCompile(ErrorReporter reporter, ReadOnlyCollection<stri
}

/// <summary>
/// Perform any required cleanups after generating code with <c>Compile</c> and <c>EmitCallToMain</c>.
/// Perform any required processing after generating code with <c>Compile</c> and <c>EmitCallToMain</c>.
/// </summary>
public virtual void OnPostCompile() { }
public abstract bool OnPostCompile(string dafnyProgramName, string targetFilename, TextWriter outputWriter);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also targetFilename should probably be targetDirectory, since you're passing targetPaths.SourceDirectory for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// <summary>
/// Remove previously generated source files. This is only applicable to compilers that put sources in a separate
Expand Down
7 changes: 4 additions & 3 deletions Source/DafnyDriver/CompilerDriver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -535,8 +535,6 @@ public static async Task<bool> CompileDafnyProgram(Program dafnyProgram, string
Contract.Assert(hasMain == (callToMain != null));
bool targetProgramHasErrors = dafnyProgram.Reporter.Count(ErrorLevel.Error) != oldErrorCount;

compiler.OnPostCompile();

// blurt out the code to a file, if requested, or if other target-language files were specified on the command line.
var targetPaths = GenerateTargetPaths(options, dafnyProgramName);
if (options.SpillTargetCode > 0 || otherFileNames.Count > 0 || (invokeCompiler && !compiler.SupportsInMemoryCompilation) ||
Expand All @@ -545,7 +543,7 @@ public static async Task<bool> CompileDafnyProgram(Program dafnyProgram, string
WriteDafnyProgramToFiles(options, targetPaths, targetProgramHasErrors, targetProgramText, callToMain, otherFiles, outputWriter);
}

if (targetProgramHasErrors) {
if (targetProgramHasErrors || !compiler.OnPostCompile(dafnyProgramName, targetPaths.SourceDirectory, outputWriter)) {
return false;
}
// If we got here, compilation succeeded
Expand Down Expand Up @@ -608,6 +606,9 @@ public override string PublicIdProtect(string name) {
public override void Compile(Program dafnyProgram, ConcreteSyntaxTree output) {
throw new NotSupportedException();
}
public override bool OnPostCompile(string dafnyProgramName, string targetFilename, TextWriter outputWriter) {
throw new NotSupportedException();
}

public override void EmitCallToMain(Method mainMethod, string baseName, ConcreteSyntaxTree callToMainTree) {
throw new NotSupportedException();
Expand Down
24 changes: 12 additions & 12 deletions Source/DafnyRuntime/DafnyRuntimeGo/System_/System_.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,47 +4,47 @@
package _System

import (
_dafny "dafny"
os "os"
_dafny "dafny"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

os "os"
)

var _ _dafny.Dummy__
var _ = os.Args

type Dummy__ struct{}


// Definition of class Nat
type Nat struct {
}

func New_Nat_() *Nat {
_this := Nat{}
_this := Nat{}

return &_this
return &_this
}

type CompanionStruct_Nat_ struct {
}
var Companion_Nat_ = CompanionStruct_Nat_ {
}

var Companion_Nat_ = CompanionStruct_Nat_{}

func (*Nat) String() string {
return "_System.Nat"
return "_System.Nat"
}

// End of class Nat

func Type_Nat_() _dafny.TypeDescriptor {
return type_Nat_{}
return type_Nat_{}
}

type type_Nat_ struct {
}

func (_this type_Nat_) Default() interface{} {
return _dafny.Zero
return _dafny.Zero
}

func (_this type_Nat_) String() string {
return "_System.Nat"
return "_System.Nat"
}

Loading
Loading