Skip to content

lint rule omit_obvious_property_types should be suppressed on public apis when type_annotate_public_apis is enabled. #60642

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bsutton opened this issue Apr 29, 2025 · 3 comments
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. devexp-linter Issues with the analyzer's support for the linter package linter-false-positive P3 A lower priority bug or feature request

Comments

@bsutton
Copy link

bsutton commented Apr 29, 2025

dart 3.7.

The follow code triggers a warning when the lint omit_obvious_property_types is enabled.

String packageVersion = '0.1.1';

The problem is that if you also have type_annotate_public_apis enabled then removing the type will trigger the type_annotate_public_apis warning.

If type_annotate_public_apis is enabled and the variable is part of the public api then omit_obvious_property_types should be suppressed for that variable.

@bsutton bsutton added the area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. label Apr 29, 2025
@bwilkerson
Copy link
Member

@eernstg

@eernstg
Copy link
Member

eernstg commented Apr 30, 2025

There could be different ways to handle this. It seems reasonable to me to enable a lint like type_annotate_public_apis, and then ensure that there are type annotations on all public declarations.

However, it also seems reasonable to me if a developer or an organization chooses to enable omit_obvious_property_types and/or specify_nonobvious_property_types in order to have type annotations on some non-local variables, and not on others, based on the characterization of the initializing expression as having an obvious type or a nonobvious type. Insisting on having return types and parameter types on all functions, and yet allowing const version = '0.1.1'; and const myNumber = 3.14; doesn't seem unreasonable to me.

Yet another reasonable approach would be to have type annotations on public declarations, no exceptions, and then having type annotations on private non-local variables if and only if the type is nonobvious.

An interesting corner case is a public member of a private class/mixin/etc, including the case where said class is accessible to other libraries because of a typedef — is that member public?

We could easily come up with additional reasonable policies, there's no obvious end in sight.

Consequently, we might want type_annotate_public_apis to stop reporting non-local variables if omit_obvious_property_types is enabled, or we might want omit_obvious_property_types to stop reporting public variables when type_annotate_public_apis is enabled, and there might be additional policies as time goes by, involving these lints and/or others.

If type_annotate_public_apis is enabled and the variable is part of the public api then omit_obvious_property_types should be suppressed for that variable.

That is one possible way to enable those two lints to coexist, but not the only reasonable one.

I suppose the ability of one lint to change the behavior based on the enabledness of another lint is a new kind of behavior. WDYT, @bwilkerson, would that be feasible? Does it generalize properly to all the situations where lints are being checked?

It does certainly happen now and then that there is a request for lints to be configurable. One difficulty with this is that there is no end to the amount of smarts we could add to lints (or that we could specify for specific combinations of lints).

The simplest way to get the requested behavior would probably be to create a completely separate lint omit_obvious_private_property_types, which would never flag a public variable. Similarly, a variant of type_annotate_public_apis could be created which would never flag a variable.

@bwilkerson
Copy link
Member

I suppose the ability of one lint to change the behavior based on the enabledness of another lint is a new kind of behavior.

I can't think of any existing lints whose behavior is changed by the enablement of another lint.

There's already a way for a lint to discover whether another lint is enabled, so this wouldn't be difficult to implement, but the hard part here would include

  • Defining reasonable semantics. Take the case in question as a perfect example: there isn't a single reasonable answer for what it should mean if both lints are enabled.
  • Explaining the semantics to users. I'm very concerned that we not build a system whose behavior is hard for the user to predict. I think it would be incredibly difficult for users if the description of a lint L1 ended up looking something like "by itself, it behaves this way, but if L2 is enabled, then it behaves that way, and if L3 is enabled it behaves some other way, unless both L2 and L3 are enabled in which case it behaves in a completely different way".
  • Maintaining consistency. Every semantic would potentially need to be implemented in both lints and those implementations would need to be kept in sync. There's no guarantee about the order in which the lints are run, so the implementation couldn't take advantage of that. (The case in question is likely an exception, but I could see cases that aren't.)

There are probably other problems that I'm not thinking of at the moment, but those are enough to make me think that we don't want to go down this road.

It does certainly happen now and then that there is a request for lints to be configurable.

Yep. And we've consistently said "no" to this kind of request. Even though there are cases where some level of configurability would admittedly be useful, allowing configurability has a tendency to exponentially increase both the complexity of the tooling from a user's perspective and the maintenance cost for the tool maintainers. I believe that in the long run it ends up being more expensive than it's worth.

In my mind, allowing interactions between lint rules has the same cost / benefit analysis. It just wouldn't be worth it in the long run.

... create a completely separate lint ...

That is a simpler way, but that also places an extra burden on users (having to understand the differences between the variants and choose the right combination) as well as a mantenance cost for us.

The simplest is probably to suggest that users either (a) don't enable both rules, or (b) use an ignore comment in cases where the rules disagree.

@bwilkerson bwilkerson added devexp-linter Issues with the analyzer's support for the linter package linter-false-positive P3 A lower priority bug or feature request labels May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. devexp-linter Issues with the analyzer's support for the linter package linter-false-positive P3 A lower priority bug or feature request
Projects
None yet
Development

No branches or pull requests

3 participants