API: Reduce parameters for AstContext::emit_lint and add docs

[xFrednet]

This came up in the discussion of #189. I believe the previous design was the correct technical implementation, I agree that it's quite verbose and potentially confusing to new users. Now we have a bunch of documentation, how AstContext::emit_lint should be used, and it only takes three parameters for a minimal lint emission. 🎉

@Veetaha would you mind providing feedback on this PR? Since this came up in the discussion from #189

[xFrednet]

TODO @xFrednet Create an issue for this, once this PR is merged

[Veetaha]

Looks good

[Veetaha]

I'd make it this way:

struct DiagnosticBuilder {
    imp: Option<DiagnosticBuilderImpl>
}

struct DiagnosticBuilderImpl {
    // all fields are here
}

If the imp is None it means the lint shouldn't be emitted. It forces you at type level to write:

let Some(diag) = &mut self.imp { return };
// calculate the diagnostic here

When with emit_lint bool every field should have a trivially-constructible default value, and you also need to remember to put an if self.emit_lint {}.

[xFrednet]

I'm considering this solution or having everything lazy, how you proposed it here #245 (comment)

Eagerly setting message for the allowed lint may also impact performance.
I wonder if the API could then look like this which forces all users to calculate the diagnostics lazily.

// Simple case
cx.emit_lint(LINT, node, |_| "Simple message");

// For elaborate case
cx.emit_lint(LINT, node, |diag| diag.message("message").help("help"));

To implement this the closure needs to have a return type that implements some trait e.g. Diagnostic, where everything that implements Into<String> also implements Diagnostic, and the DiagnosticBuilder too.

But otherwise, the API in this form is also workable. I don't have a strong preference

I understood your comments in the feedback issue, that you found the closure rather confusing. In which case, the first option would be better. 🤔

[Veetaha]

I don't see how putting everything under an Option in DiagnosticBuilder influences the emit_lint() method signature
So what signatures are you comparing with each other?

In the snippet above (with the comments for "Simple case" and "For elaborate case") the signature of emit_lint is meant to be the same:

trait IntoDiagnostic {
    fn into_diagnostic(self) -> DiagnosticBuilder
}

impl AstContext {
    fn emit_lint(
        &self,
        lint: &'static Lint,
        node: impl Into<EmissionNode>,
        diagnostic: impl IntoDiagnostic,
    );
}

// I omit the impl bodies, I suppose it's clear how they should be implemented.
impl IntoDiagnostic for &str { }
impl IntoDiagnostic for String { }
impl<'a, F: FnOnce() -> &'a str> IntoDiagnostic for F { }
impl<F: FnOnce() -> String> IntoDiagnostic for F { }
impl<F: FnOnce(DiagnosticBuilder) -> DiagnosticBuilder> IntoDiagnostic for F { }

This way it's possible to call emit_lint both eagerly and lazily.

cx.emit_lint(LINT, node, "str");
cx.emit_lint(LINT, node, "str".to_string());
cx.emit_lint(LINT, node, || "str");
cx.emit_lint(LINT, node, || "str".to_string());
cx.emit_lint(LINT, node, |diag| diag.message("str").help("foo"));

[xFrednet]

Well, if the closure taking the diagnostic builder is only executed, if the lint is emitted. We would only instantiate the diagnostic builder, to call the closure, meaning that the fields don't need to be conditional.

I'm comparing the current signature, returning the diagnostic builder, against the proposed signature of taking a closure. Or was the suggestion an addition, where the emit_lint would take a closure for the message, but still return the builder for the user?

[Veetaha]

Nope, my suggestion was such that the emit_lint method would return (). I guess you are right, in case the closure is passed as a parameter to emit_lint then nothing optional is needed in the builder

[xFrednet]

@Veetaha Do you maybe have an idea how to solve the problem?

I couldn't find a solution. Now, I'm thinking of having two methods:

1. Context::emit_lint(<lint>, <node>, <message>)
2. Context::emit_lint_and_decorate(<lint>, <node>, <message>, <decoration closure>)

[Veetaha]

Yeah, that's a tough problem. I know, for example, reqwest_middleware::RequestInitialiser suffers from the same. This inference problem isn't specific to mutability, I tried & and by-value, but it still requires a type annotation.

I think the main problem is that rustc requires this type annotation such that your code doesn't break if a new trait implementation is added in the future for a different shape of closure.

I'll try to think more about this.

See also this reply

[xFrednet]

Thank you!

I've now implemented the second option emit_lint and emit_lint_and_decorate for comparison. It seems to work fairly well. It's a little less flexible but easier to use IMO. You can checkout the latest commit for that solution or 2204702 for IntoDiagnostic

I'll fix the CI later, for now, it's just a showcase of the two options :)

[Veetaha]

Well, I guess this approach with the trait just can't work without such a compromise. There just has to be anything that hints the type to rustc. The type of the diag parameter needs to have a type annotation, or it could be inferred from usage. E.g. it needs to be passed to a function that expects exactly &mut DiagnosticBuilder parameter like this:

|diag| {
    hint(diag)
        .note("...")
        .set_main_message("...")
}

fn hint(builder: &mut DiagnosticBuilder) -> &mut DiagnosticBuilder {
    builder
}

Or the entire closure needs to be wrapped into the hint function which is all totally unacceptable.
I think your approach with having a .decorate(closure) method is then what's best here.

What I dislike about both Context::emit_lint_and_decorate(<lint>, <node>, <message>, <decoration closure>) and Context::emit_lint(<lint>, <node>, <message>).decorate(<decoration closure>) is that the <decoration closure> is the second place where the <message> could be set, making it a bit confusing. Maybe the diagnostic builder shouldn't expose the set_main_message method? Also the name of this method is rather verbose and seems to be out of place, because there is note(), but not set_note().

I was originally commenting that constructing the message could be non-trivial and should also be in closure, but maybe that's not actually a problem? This will only matter when the lint is disabled, and there should generally be quite small amount of allows in code. So I guess your original API is fine.

Regarding the difference between emit_lint_and_decordate() and emit_lint().decordate(). The latter looks cleaner but may be harder to discover.

[xFrednet]

Maybe the diagnostic builder shouldn't expose the set_main_message method?

Yeah, I think that method should be removed. I wanted to reduce the number of arguments, but that one was a bit too aggressive.

I was originally commenting that constructing the message could be non-trivial and should also be in closure, but maybe that's not actually a problem?

I liked the theoretical freedom of your proposed solution. The main messages themselves are usually very trivial. Only a tiny fraction of Clippy's have a dynamic main message, usually it's just a string literal.

This will only matter when the lint is disabled, and there should generally be quite small amount of allows in code.

This highly depends on the lint authors and users of Marker. Currently, I only have Clippy as a reference, where most lints are allow-by-default. It can be expected most Marker lints will be warn-by-default. But if they have a higher false positive (FP) rate, they're also likely to be allowed more.

So I guess your original API is fine.

Okay, thank you for all the input! We can also adapt this later if needed.

Regarding the difference between emit_lint_and_decordate() and emit_lint().decordate(). The latter looks cleaner but may be harder to discover.

We can always create a lint to suggest the usage of .decorate() 😉

[xFrednet]

I've now added the Spanned and Identifiable traits. I think they make sense, but they required me to touch numerous files. Oh well.

I had to rebase, due to some conflicts (Sorry). It probably makes sense to wait, with the review, until the next updated. The first commits stayed the same, anything after
"Chore: Address PR Review Part 2" is new

[xFrednet]

This version now uses the AstContext::emit_lint(lint, node, msg) -> DiagBuilder() method.

I've used revert commits to not force push. Before the merge, I want to squash most commits to have a cleaner history. I'm generally happy with this rework, and especially about all the new function docs. 🎉

[Veetaha]

I'm thinking of a convention for such single-method traits. For example, it's custom to name the trait exactly the same as the method that it wraps. Here are examples from std: From { fn from() }, Clone { fn clone() }, Default { fn default() }, AsRef { fn as_ref() }. But here are small inconsistencies: IntoIterator { fn into_iter() }.

But in this case, if we named the trait just NodeId then not only it wouldn't reflect the "verb" of what the trait does, it would also conflict with the NodeId enum because they share the same namespace. Maybe as an exception for "getter-like" traits they could be prefixed with Get like trait GetNodeId { fn node_id() }, and Spanned could be named trait GetSpan { fn span() }.

This is because Identifiable and Spanned sound rather ad-hoc. For example, if in the future we add a trait for attributes() method that would return a list of attributes for the node, then how would we call it Attributesful or Attributed? Well Attributted doesn't sound so bad, but maybe another example for an imaginary children() method would it be called Childrenful? Naming is hard..

Also Identifiable sounds like a rather overly generic trait name. What if in the future we add more traits like this, and the new ones will return a new type of ID? With GetNodeId such extensibility would be trivial, because the new traits will be called Get{NewTypeOfIdHere}.

[xFrednet]

How about Has<Ty>, like HasSpan and HasNodeId? This makes it a property again. I think Spanned or Identifyable sounds a bit "better" when reading the code, but this won't really work for other things, like the attribute example you mentioned

I believe we took Spanned from the example of darling::Error::with_span.

[Veetaha]

Yeah, I remember rust-analyzer also had a bunch of traits AttributesOwner or CommentsOnwer etc. Then they renamed them to HasAttributes and HasComments etc. Idk, either of them works for me just like GetAttributes.
Anyway, I'll leave the final judgement call to you

[Veetaha]

Maybe this should be called just span (stripping the set_ prefix)? Other methods like note() don't have set_ prefix.
For consistency it makes sense to either have either of the two:

• No set_ prefix for setters. If getters are needed give them a get_ prefix, like in std::process::Command API there is args() setter and get_args() getter.
• Add set_ prefix for setters. If getters are needed give them no prefix.

The first approach looks better for builders where the bulk of the API usage is setters, and getters are almost never used or even non-existent.

[xFrednet]

I'm not sure if span() might be too generic and also too similar to the span() method from all other nodes. I would also argue that .note() is slightly different in the way that it adds a note and doesn't override an existing one 🤔.

The builder example speaks again for just calling it span(). How strong do you feel about either option?

[Veetaha]

Rather strong. I believe this builder should follow std::process::Command builder design. That type has, for example .current_dir() method that is a setter which if you call it twice, the second call will override the first one's value.

But the methods .arg(), .args() don't override the previous values, they just push new ones. And I like this API, personally. It's quite obvious and intuitive for me. I'm glad that guys from std didn't add any extra prefixes/suffixes.

[xFrednet]

Okay, I don't have a strong opinion, so I'll take your suggestion :)

[xFrednet]

This should hopefully be it. I'll squash the commits, before the merge.

Thank you for the feedback!