Lint for undocumented exceptions. #58232
Comments
bsutton
added
type-enhancement
|
Thanks for the suggestion. If I remember correctly, the original rationale for not supporting checked exceptions was a belief that having any unchecked exceptions effectively negates the value of supporting checked exceptions, and the assumption that unchecked exceptions were unavoidable. I tend to agree, but I mention it not to start a discussion about the value of checked exceptions but because I believe a similar principle is at work here and should be considered. If we did implement a lint for this, then the lint would only be effective if all of the packages that a given package depends on have also enabled the lint. Packages whose authors did not choose to enable the lint have effectively turned all exceptions into unchecked exceptions. The fact that it requires wide-spread adoption of the lint to give the lint value really does significantly diminish the value of having the lint. I suspect that you realize that and that that's why you suggested adding the lint to In the absence of widespread demand for the lint, I don't see much value in adding it. I'll leave this issue open for now so that community members can voice their opinions; if there's enough demand then we'd certainly consider adding it. |
|
My first statement would be that, regardless of the checked vs unchecked
debate, documentation on what exceptions an api throws would help improve
code quality and make the life of many developers easier.
`If we did implement a lint for this, then the lint would only be effective
if all of the packages that a given package depends'
I wouldn't agree with this statement.
Whilst the mechanism would certainly work better if dependencies
implemented it, the lint would provide intrinsic value without requiring
other packages to use it.
If we could implement my preferred recommendation (look inside the first
level of called code) then we get a certain level of documentation being
'pulled' from the dependencies even if the package doesn't implement the
lint.
As a package developer who wants to provide quality documentation the lint
would make the process of documenting exceptions easier.
I would suggest that the core reason exceptions are not currently
documented is because it's hard to do so.
This lint would essentially automate the hardest piece
of documenting exceptions (discovering what exceptions are thrown).
Whilst the inclusion in pedantic would be ideal I understand that it might
be difficult to get the google team to adopt it internally.
This however doesn't undermine the intrinsic value of the lint and I would
expect over time that anyone interested in generating quality code would
adopt the lint.
Discovering what exceptions are thrown is a constant friction point when
coding in dart so I believe that developers would be happy to adopt it as
it solves a daily problem with their own libraries.
We could also encourage the adoptions by adding it to effective dart.
The core tenant in the lint system is that it should improve code quality,
I would suggest that this lint will have a significant impact on code
quality.
I would ask that you reconsider this lint.
…On Wed, 23 Sep 2020 at 01:31, Brian Wilkerson ***@***.***> wrote:
Thanks for the suggestion.
If I remember correctly, the original rationale for not supporting checked
exceptions was a belief that having any unchecked exceptions effectively
negates the value of supporting checked exceptions, and the assumption that
unchecked exceptions were unavoidable. I tend to agree, but I mention it
not to start a discussion about the value of checked exceptions but because
I believe a similar principle is at work here and should be considered.
If we did implement a lint for this, then the lint would only be effective
if all of the packages that a given package depends on have also enabled
the lint. Packages whose authors did not choose to enable the lint have
effectively turned all exceptions into unchecked exceptions. The fact that
it requires wide-spread adoption of the lint to give the lint value really
does significantly diminish the value of having the lint.
I suspect that you realize that and that that's why you suggested adding
the lint to pedantic: as a forcing function for adoption. Unfortunately,
adding it to pedantic isn't a trivial process. The pedantic rule set is
the set of lints that are used by internal developers at Google. In order
to add a lint to that set we'd first need to convince internal developers
to adopt the lint, and that doesn't seem like a likely event. So, I don't
think we can force adoption that way. Adoption would need to happen more
organically.
In the absence of widespread demand for the lint, I don't see much value
in adding it. I'll leave this issue open for now so that community members
can voice their opinions; if there's enough demand then we'd certainly
consider adding it.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#58232>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OEZHTQFAJSC3DR3BGTSHC7OVANCNFSM4RVC6GPQ>
.
|
|
Thanks for the thoughtful conversation! I see where Brian is going with the potential limitations if such an analysis were not universally enforced but I'm pretty compelled by @bsutton's observations and the general motivation. If nothing else, it seems like this could improve package documentation. I'm in favor. 👍 |
|
If the analyzer can actually analyze which methods throw which exceptions, then it would be possible to only warn about undocumented exceptions on API boundaries.
I wouldn't worry about code that is internal to a package, only documentation on API boundaries is really important. The big problem here is obviously correctly detecting which exceptions can be thrown. Static functions are probably reasonably easy, but virtual methods are harder. With the third item above, it might be sufficient to look at the actual interface documentation. It's too much of a false-positive generator to claim that an interface method throws an exception just because one subclass somewhere does, that one class might just be for testing and won't ever affect the real program. This can work for It would require analyzing the entire program, to follow call chains to the end, not just looking at signatures and documentation of what you are calling. If that's too much, then it's true that it only works for exceptions that are already documented as throwing. |
|
I would want to document internal method but can understand that might not be everyone's preference (but I still get to think they are wrong:) |
|
Perhaps the lint for internal functions can be optional for those who want it. But the lint for public APIs should arguably be on by default. |
|
Overall I do expect this would be infeasible to handle for a sufficient number of cases as to make it trustworthy.
Adding more in depth structure to DartDoc would be a departure from our current idioms and user expectations. This is still pretty readable, but I would strongly oppose any solutions which lead us towards The transitive behavior here has potential implications for evolving packages in the ecosystem - the dependencies present when we analyze a package to produce the lints may not match the dependencies present for a downstream users. This pushes towards a place where we need to model any change in exception as a major version bump (which one could certainly argue is a good thing). As a consequence we could have higher ecosystem churn - if one of my dependencies changes a thrown exception, even if that doesn't impact me directly, I can't have a wide constraint on that dependency, because my documentation is forcibly tied to a single major version of my dependency's documentation. If we do decide to pursue this, we should treat subtypes of |
|
@natebosch thats for the response
It would seem that this scenario could be handled via the existing 'ignore lint' mechanism. void a(int b)
{
if (b == null) throw Bad;
}
void c()
{
// ignore: undocumented_throws Bad
a(1);
}As to the prose I supposed the following might work: /// If you don't pass [b] then we will throw [Bad].
void a(int b)
{
if (b == null) throw Bad;
}
Mentioning the exception by name in square brackets could be considered sufficient for the linter to be happy.
I agree that this is needs some thought.
This very issue is what brought me here. Firebase made a breaking change as to what exceptions were being thrown and I had no idea that I had a problem. Fortunately the code didn't make it into production.
This is really no different to a change to an api that you aren't using. I doubt that exceptions change any more frequently than apis (in fact I suspect they change less often) as such I wouldn't expect this to create any great amount of churn and the churn it does create will be valid and necessary.
I think I agree with this. If users felt otherwise it could be implemented as an alternate lint. |
|
I think we should also allow documentation to target a base exception class. class BaseException implements Exception {}
class DerivedException extends BaseException {}
/// There is no escaping the fact that we throw [BaseException]
void main()
{
throws DerivedException();
}I would consider this lazy documentation but we probably need to cater for everyone. We could have a lint that controls whether this is allowed or explicit declarations of each exception is required. Explicit declarations of every exception can become very noisy. |
|
This is a pain point for me. Not sure what the best solution is for dart, but I personally am missing my checked exceptions I am used to. |
|
After some Flutter and Dart experimenting I can feel empathy with this issue, caused mostly by poor documentation. I miss checked exceptions, however I know they don't belong to dart. Given that I would like to ask if tracing exceptions by analyzing the throw keyword is feasible in Dart Lint or Dart itself. Example: We know that We know that This might be an optional rule. Since Dart seems to differentiate between This could be a warning or just an informative list of exceptions that each function throws, like mouse hovering a function and giving the list. I don't know if this could be ever be achieved however I think that this issue really needs some care or else what will happen is: programmers will not handle exceptions for a foolish reason, they don't know which exceptions to catch. Even if we might not know why those exceptions are being throw, since that's the documentation role, at least we are aware that the program might throw X, Y, Z exception. |
|
Any updates on this topic? Would love to see something like this as lint rule. |
|
I think with the rules from @lrhn's comment and the ability to use an |
Problem
ExpectationsDart tool should answer these 2 questions with the least and acceptable effort in both create-site and use-site. Solutions?Honestly, I don't have any solution in mind yet. I also see that all proposed solutions above don't satisfy my expectations. |
|
It's better to use annotations for that. @throws(BadRequestException(), FatalError(), SomethingWentWrongExeption())
abstract class TimeApiService {
Future<TimeResponse> getTime();
} |
|
@subzero911 Example from Version: /// Returns the primary version out of [versions].
///
/// This is the highest-numbered stable (non-prerelease) version. If there
/// are no stable versions, it's just the highest-numbered version.
///
/// If [versions] is empty, throws a [StateError]. |
|
Regardless using annotation or documentation, we are converting all runtime exceptions to the problem of checked exceptions, which force developers to add something that should be done by compiler or language tools. It only changes from method declaration (in Java) to other places (annotation or documentation). Doesn't make sense to me :-). To me, compiler or language tools have the responsibility to tell me:
All these jobs should not be on developers' shoulders :-) |
|
It seems you are mixing lint errors with compiler errors. The developer does not need to add something in order to compile. The only way to tell the developer that he might be forgetting something, well it's telling him "Hey this is a lint error, not a obligatory one, it might be worth to have a look". Regarding your last sentence, developers should (and IMO must) have documentation on their shoulders. Developers also should check for errors if they are calling another person's code with a respective API. You shouldn't skip their instructions (documentation). |
|
@rubenferreira97 sorry for confusion, I think I need to clarify that I am talking only on the use-site (including transitive call chain). I totally agree that, we need a rule to show message (of course, configurable levels: ignore/info/warn/error) to add documentation of possible exceptions and removing impossible exceptions. But only at the method, where the I disagree to force documentation anywhere, which indirectly throws an exception. Because if a method throws an exception indirectly, this method is considered use-site. The use-site developer doesn't have responsibility to document the exception. He has the right to know about it but ignore it. "Ignore" means "rethrow automatically" like what we are doing now. |
|
Now, even if we have that rule for the method, that directly throws an exception, we should never believe that the documentation about exceptions provided by a package is the source of truth. The main reason is that, the package could ignore the rule or even doing things wrongly (you know human make mistakes). If documentation is not the source of truth, dart needs to gather this information using the source code as the truth and merge the description of the valid exception found in the package documentation. It will lead to performance problem. I am not sure if we can have any solution for the performance issue here. For the packages on pub.dev, the index file could be generated by pub.dev, so localhost can ignore indexing for the parts found from a trusted place like pub.dev. |
I disagree with that. Two points often brought up around this topic are:
Both of these are perfectly valid since while the compiler may know what could happen, the programmer (usually) knows what will happen, and why it's happening, and what the real behavior should look like. If you want to ignore that, that's fine, but I wouldn't say that's the popular opinion. After all, Java is one of the only mainstream languages with checked exceptions and the rest of them seem to work just fine.
Maybe I'm not following here, but this is where you kind of lose me. Say I have this: E a<E>(List<E> values, int index) => values[index];
int b(int index) => a([1, 2 ,3], index);
void main() => b(4);As you can see, this will throw an
E? a<E>(List<E> values, int index) {
// By checking for bounds, we avoid the error below
if (index >= values.length || index < 0) return null;
else return values[index]; // ignore: undocumented_error
}
/// We don't want this to be null, so we'd rather throw an error.
///
/// Throws an [IndexError] if the index is out-of-bounds.
E a<E>(List<E> values, int index) => values[index];
/// Calls [a] with a simple list of ints.
///
/// Since this list is so small, this will result in an [IndexError] If [index] is more than 3.
int b(int index) => a([1, 2 ,3], index);
// ^ LINT: Possible IndexError at line X using `a`, but no corresponding documentation was found.
/// Call this function with a number smaller than 3.
///
/// Notice how I don't have to call out the error by name here. That's because I'm no longer
/// documenting a runtime error, just the normal behavior of my function.
int b(int index) {
if (index < 0 || index > 3) return 0;
else return a([1, 2, 3], index); // ignore: undocumented_error
}Now, |
|
Basically, I see that we all don't have controversial on direct exceptions. The controversial is on transitive exceptions. For transitive exceptions, I think this way:
So the final result is that, dart will show the truth (real call chain from source code) plus the additional documentation found from the call chain. 2 reasons why I couldn't consider documentation is the source of truth:
Could you please prove that I am wrong with the 2 reasons above? I am happy if someone can prove that, then the community could go on the easier path. Java initially wants to solve this issue with checked exceptions by forcing declaring or handling "transitive" checked exceptions. And it doesn't work well, that's why modern languages learned the lesson and avoid the similar problem. Moving the place of the rule from method declaration to documentation/annotation for transitive exceptions is not the solution. |
|
To me, it's very important to distinguish errors and exceptions. Errors are like Java's And, also, all code can throw errors, it just might be We don't generally document errors being thrown, we instead document restrictions that must be checked to avoid that error, like "the Exceptions are where the caller is expected to catch and handle the exception. The way we've chosen to document exceptions in the platform libraries (decided that around the 1.0 release, some code will predate that) is to have a DartDoc paragraph starting with "Throws": ///
/// Throws [SomeException] ... if something, when something, unless something,
/// meaning something. Maybe suggested responses.(And one of the reasons we try to avoid saying "Throws [ArgumentError] if argument is bad.", is to save that syntax for exceptions. That's why errors are just written as "Argument must not be bad". Also because we don't consider changing which error is thrown a breaking change.) |
|
Yep, I like that there's that distinction. My hope is that such a lint will make people think not just about what errors they haven't documented, but also about how they can handle it themselves. I tried to show it a bit in my example too, where for each step I either documented it or added in |
|
Adding to @lrhn comment:
I wanted to express my support for the idea of implementing a lint for undocumented exceptions. Properly documenting exceptions in code is crucial for improving code clarity, and maintainability, and reducing potential issues during development. By enforcing the documentation of exceptions and drawing inspiration from other languages' approaches, we can improve code quality, promote explicit error handling, and enhance the overall reliability of Dart projects. As for exceptions that never occur, I would use a "panic-like" behavior: terminating the program, or logging an unexpected occurrence. This approach emphasizes the confidence that the exception will not be thrown and helps identify unexpected scenarios during development. I would like to provide an extreme approach to answer @natebosch's question:
void c() {
try {
a(1);
} catch (Bad) {
// Perform panic-like action here, such as terminating the program
log("this is not expected to happen")
exit(1);
}
}By doing so, other developers can understand that this particular situation is not expected to happen. That way you don't need to ignore the linter. In Dart, it can be challenging to determine all possible exceptions that a function might raise without thoroughly reading its documentation or analyzing its implementation. Having IDE support or a compiler that can automatically identify and suggest exception handling for functions would indeed be helpful. However, traversing all inner calls of all functions to determine potential exceptions can be a computationally expensive process, especially in large codebases. One possible solution to address this issue is by standardizing exception documentation and making it more explicit in function signatures. By providing clear and standardized documentation about the exceptions that a function may throw, IDEs could potentially offer autocomplete or suggestions for exception handling, enhancing developer productivity and reducing the likelihood of missed exceptions. Drawing from the experiences of languages like Go and Rust, where the exact exception type is not as critical as ensuring explicit error handling, it may be beneficial to focus on encapsulating error messages within exceptions. This approach allows for flexibility in modifying the error message without breaking client code, as long as the exception type remains the same. In contrast, requiring the specification of exact error types, as seen in Java, can sometimes lead to verbosity and increased complexity. By adopting practices that prioritize standardized exception documentation and promoting explicit error handling, we can improve the developer experience, code maintainability, and overall code quality. Thank you for considering this suggestion. |
|
When considering the annotation idea, instead of an annotation, what about a generic typedef? // The typedef is No-op, and useful only for the analyzer
typedef Throws<ResultT, ErrorT> = ResultT;Then instead of: int fn() {
throw FormatException();
}We'd write: Throws<int, FormatException> fn() {
throw FormatException();
}This doesn't impact the function signature. Throws<int, Error> Function() callback;As for when throwing multiple types, we can use records: Throws<int, (FormatException, UnsupportedError)> Function() callback;This even supports more advances cases, such as a function returning a function: Throws<Throws<void, FormatException> Function(), StateError> callback;
callback = () {
if (something) throw StateError();
return () {
if (something) throw FormatException();
};
}
|
|
@rrousselGit We still need to document why the exceptions are being throw so this proposal just ends up in documenting the exceptions twice. |
|
Making sure exceptions are explicitly thrown or handled is the responsibility of the type system though, not comments. |
|
I would love to see some results rising from all the discussions. Maybe we don't need the full fledged solution that hits it all. Maybe we can start rather simple with a lint that is limited to only scan the body of a function for |
|
Sounds like a good starting point.
…On Mon, 21 Oct 2024, 9:40 pm MisterMirko, ***@***.***> wrote:
I would love to see some results rising from all the discussions. Maybe we
don't need the full fledged solution that hits it all. Maybe we can start
rather simple with a lint that is limited to only scan the body of a
function for throw statements and advise the programmer to document the
exception.
—
Reply to this email directly, view it on GitHub
<#58232>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OGYXY3O6OBPWG2MJETZ4TKY5AVCNFSM6AAAAABQJ3JACWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMRWGI4TEMBWGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
devoncarew
added
devexp-linter
bwilkerson
added
area-devexp
The dart language supports exceptions but not checked exceptions.
The lack of check exceptions means that developer must rely on documentation to determine what exceptions a method will throw.
If you look at a significant portions of code published on pub.dev, developers are simply not documenting the exceptions that are thrown.
Documenting exceptions is particularly error prone as it requires the developer to explore each called api to determine the set of exceptions the api throws.
Of course given that most api's are not documenting their exceptions the result is that a developer must examine thousands of lines of code to determine what exceptions are being thrown.
This of course results in the developer not documenting their own api as its simply too much effort and worse they simply are not doing the necessary work to handle exceptions appropriately.
The lack of documentation then cascades up into the next level of code and so on.
The end result is that much of the code published on pub.dev is of questionable quality.
This problem could largely be resolved by providing a lint rule for pedantic that warns if an api throws an exception.
By adding this link to pedantic we would see a very rapid improvement on the quality of code on pub.dev for the following reasons.
Essentially we end up with a cascade affect and everyone benefits.
Here is an example of how I would see the workflow.
Linter:
Once I correct the above lints I should then see:
Lint error:
I see two alternatives for how the linter inspects the code.
The 2nd alternative would see a much faster improvement in code on pub.dev as it would:
The result is that the lint would essentially cascade both 'up' into the developers own code and 'down' into third party projects resulting in an improvement in quality of all dart code.
This lint looks (in my ignorant opinion) fairly easy to implement.
The benefits to the community however will be huge.
The text was updated successfully, but these errors were encountered: