`dart analyze` should gracefully handle analysis options formatting errors about sdk

[DanTup]

Some slight changes to what I noted above, with the latest change errors in analysis_options/pubspec are prioritized and shown first with an additional message:

Analyzing myapp...

Errors were found in 'pubspec.yaml' and/or 'analysis_options.yaml' which might result in either invalid diagnostics being produced or valid diagnostics being missed.

  error • analysis_options.yaml:3:1 • Expected ':'. • parse_error

Errors in remaining files.

  error • lib\main.dart:1:16 • A value of type 'Null' can't be returned from the function 'foo' because it has a return type of 'int'. • return_of_invalid_type

2 issues found.

[lrhn]

Should probably catch the error and report it (more) gracefully. Don't know if this should be handled in the dart executable or at the analyzer end, but printing null suggests it's the CLI end that doesn't handle server errors.

(If the YAML parser could be made to recover from errors, it might be possible to give more precise errors too, but that depends on the yaml package.)

[parlough]

\cc @DanTup Since I think you recently added some handling for when the analysis server crashes.

[DanTup]

I think there are two behaviours here that may be questionable, but I don't know what the desired behaviour is for each (cc @bwilkerson).

1. What should the server do with an invalid analysis_options? Currently it's throwing an unhandled exception while trying to set the analysis which is sent back to the client with a stack trace. This seems wrong to me, but I don't know if the error is considered fatal or not.. If so, it should probably report the error in a more friendly way, and if not, should it just continue to set up the analysis roots and analyze as if the invalid file (or parts of) didn't exist?

2. If the server does return an unhandled error to dart analyze, how should it be presented? The original comment above suggests it should be more user friendly (presumably not be a raw exception/stack trace) but if this is an edge case and indicates a bug in the server, I wonder if this information is useful, because it results in a much better bug report than if it was not included. I think this might depend on the kind of exception.. For example the above code is OptionsFormatException which suggests we know the issue here is an invalid analysis_options.yaml and could provide a useful message without the full exception. But if it was something like a RangeError, I think the raw exception could be useful (in which case, maybe dart analyzes behaviour is fine here, and the server should handle exactly what's in the server error responses).

[bwilkerson]

What should the server do with an invalid analysis_options? Currently it's throwing an unhandled exception while trying to set the analysis which is sent back to the client with a stack trace. This seems wrong to me ...

I agree. I don't think the server should be returning an exception in this case.

The general design of the server is to recover from errors as cleanly as possible, notifying the user of the error with enough information that the user can fix the problem.

I don't know what the limitations of the yaml package are in this case, but server should, to the best of its ability, create a diagnostic indicating the location and nature of the problem. If it gets back a partial view of the file then we ought to use as much of the file as we get and ignore the rest, and then proceed with analysis.

Ideally, the dart analyze tool would then look at the diagnostics that are returned, and, if there are diagnostics reported against either any analysis_options.yaml or pubspec.yaml files it should print

1. a notice indicating that the rest of the analysis might not be correct because of those issues,
2. the diagnostics that might invalidate the rest of the results,
3. a header indicating that the questionable diagnostics follow
4. the rest of the diagnostics (or notice that no other diagnostics were found if that's the case).

If the server does return an unhandled error to dart analyze, how should it be presented?

Given that this should only happen as a result of an internal bug that the server couldn't recover from, I think it's reasonable for the command-line tool to print out the message and stack trace, prefixed by a message explaining that this is an internal error and asking the user to report it.

[DanTup]

It seems we already handle generating diagnostics for the case above, the issue is just that the server crashes before it gets there (during setting the roots). There are many places already handling these errors, but not path in the stack trace above.

I've started a change at https://dart-review.googlesource.com/c/sdk/+/371861 which:

1. Changes the text for unhandled errors from

   Error from the analysis server: (error)
   to
   An unexpected error was encountered by the Analysis Server.
   Please file an issue at https://github.com/dart-lang/sdk/issues/new with the following details:
   (this text was copied from another place in dartdev that handles top=level errors)

2. Handles the parse error by returning an empty map for the options (as other places already did)
3. Adds a suffix to analysis_options parse errors:

   Parse errors in analysis_options.yaml may result in other incorrect diagnostics.

The result is that dart analyze with the above analysis_options.yaml looks like this:

I'm not sure if we want to just mash that text on to the end of the error like that though - it's not what was suggested above, but it does mean that it appears in the clients instead of only dart analyze (where the same problem exists). We can discuss this on the CL though.