Skip to content

Commit

Permalink
refactor/rename: don't stop because of "soft" errors
Browse files Browse the repository at this point in the history
Because go/types is slightly more strict than gc about certain "soft"
errors (ones that aren't necessary to interpret a Go program), gorename
rejects programs that compile under gc.  This change relaxes gorename's
error checks so that they are weaker than gc's.

This is a workaround for issue golang/go#14596 in gorename,
whose underlying problem is issue golang/go#8560 in gc.

Fixes golang/go#14596

Change-Id: Ica5006c2376c0564a575224269093c1497348ee6
Reviewed-on: https://go-review.googlesource.com/29853
Reviewed-by: Brad Fitzpatrick <[email protected]>
  • Loading branch information
adonovan committed Sep 27, 2016
1 parent c2ef61f commit fc2b74b
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 1 deletion.
36 changes: 35 additions & 1 deletion refactor/rename/rename.go
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,41 @@ func loadProgram(ctxt *build.Context, pkgs map[string]bool) (*loader.Program, er
for pkg := range pkgs {
conf.ImportWithTests(pkg)
}
return conf.Load()

// Ideally we would just return conf.Load() here, but go/types
// reports certain "soft" errors that gc does not (Go issue 14596).
// As a workaround, we set AllowErrors=true and then duplicate
// the loader's error checking but allow soft errors.
// It would be nice if the loader API permitted "AllowErrors: soft".
conf.AllowErrors = true
prog, err := conf.Load()
var errpkgs []string
// Report hard errors in indirectly imported packages.
for _, info := range prog.AllPackages {

This comment has been minimized.

Copy link
@dchapes

dchapes Feb 23, 2017

Need to check err before referencing prog here otherwise it will panic as it did for a recent reddit user.

This comment has been minimized.

Copy link
@davidrjenni

davidrjenni Feb 23, 2017

Contributor

@dchapes, are you working on a fix? Otherwise I'm happy to submit one.

This comment has been minimized.

Copy link
@dchapes

dchapes Feb 23, 2017

@davidrjenni no I am not. I'm just doing the minimum effort that the reddit poster should have done; making a code-line comment is way easier than filling an issue :).

This comment has been minimized.

Copy link
@dmitshur

dmitshur Feb 23, 2017

Contributor

It is easier, but it's not the process that Go uses to track and discuss issues. See https://github.com/golang/go/blob/master/CONTRIBUTING.md#filing-issues.

This comment has been minimized.

Copy link
@davidrjenni

davidrjenni Feb 23, 2017

Contributor

This comment has been minimized.

Copy link
@dchapes

dchapes Feb 23, 2017

@shurcooL I am aware. Tell the reddit poster.

I could care less about this "issue"... I happened upon this while doing minimal effort to reply on reddit and figured I'd hit "Add comment" rather than do nothing. If it went unseen so be it. I wasn't going to spend my time filling someone else's issue.

if containsHardErrors(info.Errors) {
errpkgs = append(errpkgs, info.Pkg.Path())
}
}
if errpkgs != nil {
var more string
if len(errpkgs) > 3 {
more = fmt.Sprintf(" and %d more", len(errpkgs)-3)
errpkgs = errpkgs[:3]
}
return nil, fmt.Errorf("couldn't load packages due to errors: %s%s",
strings.Join(errpkgs, ", "), more)
}
return prog, err
}

func containsHardErrors(errors []error) bool {
for _, err := range errors {
if err, ok := err.(types.Error); ok && err.Soft {
continue
}
return true
}
return false
}

// requiresGlobalRename reports whether this renaming could potentially
Expand Down
23 changes: 23 additions & 0 deletions refactor/rename/rename_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1058,6 +1058,29 @@ type J interface {
}
var _ = I(C(0)).(J)
`,
},
},
// Progress after "soft" type errors (Go issue 14596).
{
ctxt: fakeContext(map[string][]string{
"main": {`package main
func main() {
var unused, x int
print(x)
}
`,
},
}),
offset: "/go/src/main/0.go:#54", to: "y", // var x
want: map[string]string{
"/go/src/main/0.go": `package main
func main() {
var unused, y int
print(y)
}
`,
},
},
Expand Down

0 comments on commit fc2b74b

Please sign in to comment.