Unify Lint Policy?

[Madoshakalaka]

#4118 introduced a special class of deprecation lints that are warnings on nightly and are hard errors otherwise.

Conflicts to be resolved

• This class of lints doesn't interact with our yew_lint cfg flag at all.
• Our other html lints (like missing href attribute on an <a>) are disabled by default and enabled on a opt-in bases. While the deprecation lints are always on.

My Opinion

Our previous html lints are too quiet. A user has to pass the cfg flag and be on nightly. The fact that the yew_lint flag isn't documented anywhere doesn't help. Honestly I have never used the flag during my years of using Yew. Mostly not remembering it existed, despite being on nightly all the time.

Action

A possibility is we handle these classes of lints seperatly (because they indeed are about different things. One is about deprecated yew patterns and the other is about html standards). Then we don't need to change to the code. We just need to document the yew_lint flag better. We can also consider adding the flag to the yew-trunk-minimal-template.

I want to get more input on this before doing anything. Tagging some people here:

@ranile who created the yew_lint cfg flag
@its-the-shrimp who worked on the String vs AttrValue lint
@WorldSEnder active maintainer
@kirillsemyonkin who said "matt why is this a hard error"
@stifskere who suggested the warning plus hard error behavior

[ranile]

This is a painful area in proc_macros in general because to my knowledge lints are not possible on stable. cfg_lint called a nightly-only API

This might've changed in the years since the lints were added so it's worth reevaluating

[WorldSEnder]

Here's a crazy idea, that might just work. Instead of emitting a hard error and a lint, we could emit a small helper function

#[deprecated = "why not to use this syntax"]
fn deprecated<T>(t: T) -> T { t }

and make sure to call this in the emitted code. I agree that a hard error for deprecated syntax is bad UX. The old code is functionally fine and stylistic choices should not lead to hard errors. I'm actually not 100% convinced that all these lints are air-tight, any false positive would lead to unavoidable compile errors that can't be ignored. The above could be locally or globally ignored with a allow(deprecated).

All of this under the assumption that no other diagnostics work, the underlying Diagnostic are still nightly only.

[Madoshakalaka]

I think I saw this idea somewhere. I can do some experiments in this direction.

On the air-tightness, my (overly-optimistic maybe) hope is to get some feedback on existing codebases before this is released. Nonetheless, I'll be paying attention and fixing the lints as problems arise. On the upside, these worked well on all our examples and code snippets on the website.

[Madoshakalaka]

false positive would lead to unavoidable compile errors that can't be ignored

This is an overstatement though. Internal types html! uses are all in the public API. One can also manually construct them as a last resort. This is what I encountered and did during the PR when a pattern in a unit test is no longer valid

[WorldSEnder]

One can also manually construct them as a last resort.

That is true. Consider though that fragments also scope keys to their children only, for example

let menu = html! {
  <>
    <p key="header">{"Menu Header"}</p>
  </>
}

will not "conflict" with other elements if you later use it as

html! {
  <div>
     <p key="header"></p>
     {menu}
     <p key="footer"></p>
  </div>
}

If you get rid of the "unnecessary" fragment, the keys will conflict though.

This is of course a bit of a silly example, but the point is that precise control over the vdom should be possible and is not always "bad practice" just because the lint declares it as such and should not mean you have to reach for the rust API imo. So check_unnecessary_fragment already is a bit too eager, imo.

In any case, the main point I want to make is not that the lint has to be perfect and might be imprecise. It's that the even if the lint works exactly as intended, a hard error is false. Using the old syntax leads to working code. Sure, you can and should migrate to the shorter code for readability and it's good to have the support. A bit of nagging is fine. But at the end of the day, the old syntax still means the "same" thing, just with more verbosity and is not an error.

[ranile]

The issue I see with deprecations is that it's not possible can't control lints. It's either all or nothing. There's cases where one might want to disable a lint app-wide but that's not possible with deprecated functions alone

This is an overstatement though. Internal types html! uses are all in the public API. One can also manually construct them as a last resort

That sounds like asking a lot from people to disable lints. The only reason I ever had to that was to use custom elements

[Madoshakalaka]

@ranile @WorldSEnder I created #4129