Shorten expression strings in assignment messages

[sonalmahajan15]

This PR shortens the expression strings in assignment part of the error messages. For example, the expression x = s.foo(longVarName, &anotherLongVarName, "abc", true) is shortened to s.foo(...) to offer better readability. The changes have also have been performance evaluated internally.

[codecov]

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (43b26d7) 89.24% compared to head (7ca80e6) 89.18%.

Files	Patch %	Lines
assertion/function/assertiontree/backprop.go	40.00%	8 Missing and 4 partials ⚠️
util/asthelper/asthelper.go	80.32%	7 Missing and 5 partials ⚠️

Additional details and impacted files

@@                              Coverage Diff                              @@
##           sonalmahajan15/fix-backprop-error-message     #153      +/-   ##
=============================================================================
- Coverage                                      89.24%   89.18%   -0.07%     
=============================================================================
  Files                                             54       55       +1     
  Lines                                           9087     9156      +69     
=============================================================================
+ Hits                                            8110     8166      +56     
- Misses                                           820      825       +5     
- Partials                                         157      165       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[yuxincs]

So this means the result will be incorrect if I have code like (m["("]), right? since it's checking for specific characters.

I'm thinking that we probably shouldn't post-process the expr string, but we should simply work on the AST nodes or printer logic. But after a bit of thoughts they all seem unachievable:

(1) traverse the AST node and remove the argument if it's too long and then print

We shouldn't modify the AST nodes since they are shared with other analyzers. Maybe https://github.com/go-toolsmith/astcopy to deep copy the AST nodes?

(2) Change the printer logic

The https://pkg.go.dev/go/printer package doesn't provide customizations unfortunately, so we'll have to copy the printer logic in NilAway and implement some customizations. But that seems too much of a burden to maintain.

Thoughts?

[sonalmahajan15]

That's a good point. We could always handle quotes as well in the string processing approach. But I get the broader point that this may require us to deal with many corner cases. As you pointed out, the two options discussed above also have their own issues. So, as we discussed internally, I have updated the shortening logic to do a light-weight AST traversal and printing, with defaulting to printer package for all unhandled cases. I have tested it internally and seems to be working fairly well.

[sonalmahajan15]

Also, added many test cases around (m["("]) :)

[yuxincs]

Couple of suggestions, but no real blockers (the logic LGTM, nice work!)

[yuxincs]

Again, since strings.Builder actually satisfies io.Writer interface, let's take io.Writer instead for better generality (i.e., "accept interface, return structs")

Two nits:

• We can actually drop the Helper suffix in the name since it's pretty common to have an unexported internal helper function for the exported API :)
• We probably want to follow the function signature of printer.Fprint for consistency (i.e., func printExpr(writer io.Writer, fset *token.FileSet, e ast.Expr) error).

[sonalmahajan15]

That's a good suggestion! Updated the logic to incorporate the above points.

[yuxincs]

Suggested change

	if !isShortenExpr {
	astExprToString(e, pass)
	}
	if !isShortenExpr {
	return astExprToString(e, pass)
	}

Right?

Actually, we can get rid of the astExprToString helper function, since strings.Builder actually satisfies the io.Writer interface.

This means we can simply do the following:

func PrintExpr(....) (string, error) {
  builder := &strings.Builder{}

  if !isShortenExpr {
    err := printer.Fprint(builder, pass.Fset, e)
    return builder.String(), err
  }

  err := printExpr(builder, pass.Fset, e) 
  return builder.String(), err
}

If we really don't want to handle the errors, we can offer a MustPrintExpr variant that panics on error :) I think we should generally keep the errors internally no matter what, and at the API level, if we really want to offer the conveniences, we offer the Must variants.

[sonalmahajan15]

Nice! Did not know that we could use printer.Fprint() directly here. Updated the code to reflect this. Also, I agree that we should handle errors when possible. So incorporated that approach in the code.

[yuxincs]

Here, we can keep using the buffer instead of "calling a function that creates a new buffer and returns the result and then writes the result to the old buffer".

return printer.Fprint(writer, fset, e)

[sonalmahajan15]

Done :)

[yuxincs]

We can still use io.WriteString (https://pkg.go.dev/io#WriteString) that handles conversions of strings to underlying bytes instead of manually constructing them. This should provide better utf-8 compatibility for languages other than English :)

(actually, this reminds me, can we add a case where the expression contains non-English characters?)

We can use fmt.Println("Hello, 世界") and fmt.Println("世界") from Go's official docs :)

[sonalmahajan15]

Ah nice, this is convenient! Updated the code. Also, added a test case for non-English expressions.

[yuxincs]

LGTM! Great work 🎉