Refactor the report package #390

mcy · 2024-12-11T00:20:45Z

This PR adds a few new features to the report package:

report.Tag replaces the functionality of being able to remember the type of a report.Diagnose. As such, report.Diagnose no longer embeds error.
report.Message is now the correct way to set a message for a diagnostic.
report.Diagnostic no longer exports its fields, and report.Annotation (now report.snippet) is not exported at all.
report.ICE is a real level instead of being hacked-in with a separate field in Diagnostic.
report.DiagnosticOption is now an interface.

In the sequel, I will add functionality for specifying suggested fixes in a diagnostic.

jhump

I was nearly through a review when you heavily overhauled it and force-pushed. So some of these comments (if they all successfully show up) are likely all marked "outdated". I'll have to do a second pass in the morning to see if there's anything else or new to add (and to close any comments that clearly don't apply).

Edit: Looks like there was only one comment that is outdated. I still want to take another pass tomorrow morning.

The one part I had not reviewed was serendipitously the part that is now gone -- the diff computation. (I saved that for last because I was not looking forward to it. So, just foreshadowing for if/when you open a future PR with that code: I'm highly skeptical it's worth adding that now, if ever.)

jhump · 2024-12-11T20:49:32Z

experimental/parser/diagnostics_token.go

@@ -68,34 +62,31 @@ type ErrUnmatched struct {
 }

 // OpenClose returns the expected open/close delimiters for this matched pair.
-func (e ErrUnmatched) OpenClose() (string, string) {
+func (e errUnmatched) OpenClose() (string, string) {


Since the type is unexported, we should probably make this method unexported, too.

Should I take this as a general bit of style guidance from you? I have typically taken the convention that methods on unexported are exported if they are intended to be called by functions in other files. But I haven't been super consistent... and I think this is an easier convention to follow.

Yeah, maybe it's a peculiar thing specific to me 🤷

I typically default to making everything unexported, but especially for values that escape this package (like error types). Otherwise, exported functions could be called from outside the package by external code type-asserting to an interface or via reflection. Just seems like better encapsulation to not have to worry about that (this is method is harmless, but it's particularly bad for methods that mutate the value or could be used to break invariants if called incorrectly). This seems particularly important from a backwards-compatibility story and preventing yourself from being "trapped" into maintaining things due to Hyrum's Law.

So the only times I will export things from an unexported type are:

When it's required for interop with other packages -- like fields of a struct used with "encoding/json"

When we actually want/need the type to implement an interface -- for use within this package or use outside of it (for cases where instances of the unexported type can escape)

When the type has its own helper methods and it would materially improve readability/maintainability to distinguish between the methods that other code is expected to use (exported) and those that should be private to the type (unexported).

if they are intended to be called by functions in other files

But common refactors like moving things between files or breaking up a large file into multiple files can suddenly mean you're breaking this convention. Without anything to enforce this convention (like a linter), it feels brittle and unreliable, so I'd personally just rather not.

I am fairly cognizant of unexported method names escaping. I usually pay attention to those.

I do wish there was a linter for this: enforce that all unexported types' methods are unexported, unless that type is converted into an interface with such a method within the package.

Perhaps I should write one. Can't be that hard.

experimental/parser/diagnostics_token.go

jhump · 2024-12-11T21:04:32Z

experimental/report/report.go

-
-// Notef is like [Note], but it calls [fmt.Sprintf] internally for you.
-func Notef(format string, args ...any) DiagnosticOption {
+func Note(format string, args ...any) DiagnosticOption {


If only offering one version that uses fmt.Sprintf (which I think is fine -- that's how the existing error reporting stuff in this repo works), I would name it with the f suffix, to make it clear at call sites that the string can contain format directives. So maybe rename these to Notef, Helpf, Snippetf, etc. I would then keep Snippet as a separate function that only takes the span and does not accept a format or message.

I don't really like the f prefix (for reasons of vanity: I use a spell-checker for code and it HATES the f prefix, I constantly need to add new things to my dictionary). If you insist I'll add it, but I really don't want to have separate Snippet and Snippetf functions. I would rather write Snippet(span, "").

I think the API would definitely benefit from the "f" suffix: readers would roughly know the function's contract/what to pass just based on the function name. It's a useful convention, and I'd prefer we stick with it.

As far as not having a separate Snippet function and just calling Snippetf(span, ""), that seems fine to me, but I thought I saw a fair number of usages of Snippet with no args in the parer PR. So it seems like the second function would definitely be valuable for making those calls more concise and more readable.

I don't really agree but I don't really think it's worth fighting about. :)

experimental/report/report.go

jhump · 2024-12-11T22:50:57Z

experimental/report/diagnostic.go

+// Apply implements [DiagnosticOption].
+func (t Tag) Apply(d *Diagnostic) {


I think I liked it better in prior push, when there was function that returned a DiagnosticOption. That way it shows up in Go docs under the DiagnosticOption type -- this won't, so it's much less discoverable. Having said that, I wasn't a big fan of that function being a method on Tag: it is more discoverable (when using auto-complete to look for options) for it to be a top-level function like all of the other functions that return options. So instead, how about a top-level WithTag(Tag) DiagnosticOption?

Thankfully, your discoverability concerns aren't as big of a problem as you might fear.

For one, in the package docs, Tag.Option would have never sorted under DiagnosticOption. I do wish go had a way to spotlight implementations... but here we are (Rust has the same problem but the other way around, it shows too much!). The factory functions, thankfully, do not have this problem.

Also, autocomplete does find Tag constants in scope.

We can add

func WithTag(t Tag) DiagnosticOption { return t }

if you want, but it feels redundant to me. The With prefix is also not consistent with the other option factories, and I'm not keen to add it...

Thankfully, your discoverability concerns aren't as big of a problem as you might fear.

Hmm. Those screenshots don't assuage my concerns at all. If anything, they make it more clear that report.WithTag would be better -- it would show up in the docs and it would show up to auto-complete after report. (which is the prefix that all other options use). The auto-complete screenshot doesn't make sense: what are all of those other things in the suggestion list? How would I know that I can use the tag to produce an option? The inconsistency just makes it feel incoherent.

The only reason I suggested the With prefix is because report.Tag is already taken by the type. What about this: just use string as the type of a tag and then you can have report.Tag(string) DiagnosticOption as the way to set the tag. WDYT?

I would prefer for Tag to be a type, for the sake of exported constants having a type that isn't string. But I don't think it matters too much in the end... if it turns out to be a problem we can revisit.

jhump · 2024-12-11T22:52:10Z

experimental/report/diagnostic.go

+//
+// Nil values passed to [Diagnostic.With] are ignored.
+type DiagnosticOption interface {
+	Apply(*Diagnostic)


I think we should make this unexported. That is most typical of this sort interface-option pattern. Since the argument has no exported fields, it's not valuable for another package to be able to implement an option, so might as well make its definition completely internal to this package.

Not wrong. I had contemplated merging DiagnosticOption and Diagnose... it's nice to not have two interfaces that basically do the same thing. At least, it would mean you could write report.Error(report.Message(blah)), but I would still want to keep report.Errorf.

experimental/report/diagnostic.go

jhump

I would really prefer to keep the "f" suffix on the report functions that take format directives.

Otherwise, this LGTM.

mcy added 2 commits December 10, 2024 16:12

introduce diagnostic tags

1b82092

pull ICE out as a genuine level

7bd588a

mcy requested a review from jhump December 11, 2024 00:20

mcy added 6 commits December 11, 2024 12:35

lint

ff09584

use an interface for DiagnoseOption

985df13

split off diagnostic.go

f60a48f

split off error.go

ec141f7

rename annotation -> snippet for consistency

a67c3fe

lint

82143c5

mcy force-pushed the mcy/diagnostic-info branch from aad5cf6 to 82143c5 Compare December 11, 2024 21:18

mcy changed the title ~~Add tag and suggestion support to the report package~~ Refactors in the report package Dec 11, 2024

mcy changed the title ~~Refactors in the report package~~ Refactor the report package Dec 11, 2024

mcy mentioned this pull request Dec 11, 2024

Implement a new parser on top of the new lexer #370

Merged

jhump reviewed Dec 12, 2024

View reviewed changes

cr

c0f5769

mcy requested a review from jhump December 12, 2024 19:24

cr

13983b6

jhump approved these changes Dec 13, 2024

View reviewed changes

cr

c0b0eb3

mcy enabled auto-merge (squash) December 13, 2024 19:04

mcy merged commit 8da83e7 into main Dec 13, 2024
9 checks passed

mcy deleted the mcy/diagnostic-info branch December 13, 2024 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the report package #390

Refactor the report package #390

mcy commented Dec 11, 2024 •

edited

Loading

jhump left a comment •

edited

Loading

jhump Dec 11, 2024

mcy Dec 12, 2024

jhump Dec 12, 2024 •

edited

Loading

mcy Dec 12, 2024 •

edited

Loading

jhump Dec 11, 2024

mcy Dec 12, 2024

jhump Dec 13, 2024

mcy Dec 13, 2024

jhump Dec 11, 2024

mcy Dec 12, 2024

jhump Dec 12, 2024

mcy Dec 12, 2024

jhump Dec 11, 2024

mcy Dec 12, 2024

jhump left a comment

		// Apply implements [DiagnosticOption].
		func (t Tag) Apply(d *Diagnostic) {

Refactor the report package #390

Refactor the report package #390

Conversation

mcy commented Dec 11, 2024 • edited Loading

jhump left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhump Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

mcy Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhump left a comment

Choose a reason for hiding this comment

mcy commented Dec 11, 2024 •

edited

Loading

jhump left a comment •

edited

Loading

jhump Dec 12, 2024 •

edited

Loading

mcy Dec 12, 2024 •

edited

Loading