Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2979623002:
Use messages for (some) public API errors (Closed)
|
Created:
3 years, 5 months ago by Siggi Cherem (dart-lang) Modified:
3 years, 5 months ago CC:
reviews_dartlang.org, dart-uxr+reviews_google.com, dart-fe-team+reviews_google.com Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionUse messages for (some) public API errors
not entirely ready - looking for comments.
BUG=
R=ahe@google.com
Committed: https://github.com/dart-lang/sdk/commit/a7865761d4829bc5d3cbd7da857faba0081311b4
Patch Set 1 #
Total comments: 23
Patch Set 2 : wrap - verification error still pending #
Total comments: 15
Patch Set 3 : cl comments #
Total comments: 22
Messages
Total messages: 23 (7 generated)
Description was changed from ========== Use messages for (some) public API errors BUG= ========== to ========== Use messages for (some) public API errors not entirely ready - looking for comments. BUG= ==========
Patchset #1 (id:1) has been deleted
sigmund@google.com changed reviewers: + ahe@google.com, paulberry@google.com
https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/compi... File pkg/front_end/lib/compilation_error.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/compi... pkg/front_end/lib/compilation_error.dart:17: abstract class CompilationError { Paul - I like the idea of renaming this to CompilationMessage or something like that. The reason: we will report other messages that are not errors (warnings, hints, etc). https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/b... File pkg/front_end/lib/src/base/processed_options.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/b... pkg/front_end/lib/src/base/processed_options.dart:113: void deprecated_reportError(String error) { FYI - I'm following Peter's style here, it is a transitional name to help us migrate use sites. https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/fasta_codes.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/fasta_codes.dart:73: class LocatedMessage implements CompilationError { Peter/Paul: a few questions for you here: 1- I would like to use messages.yaml for all of our errors, which means practically that we'll use LocationMessage as the basic type we pass around in the compiler and that eventually is used to produce errors publicly. To report an error we could either wrap these message objects or do what I did in this patchset. Looking ahead, it seems to me like we'll want to wrap: the public API needs to include the notion of severity (error/warning/hint/nit) and I don't believe that should go here. Especially because different tools may want to adjust the severity of messages (e.g. treat warnings as errors). What are your thoughts? do you agree with my thinking? If so, I'll update this CL and wrap LocationMessage instead. 2- How do we want to publicly expose analyzer and dart2js codes? Should we simply return the String in [Code]?
https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/compi... File pkg/front_end/lib/compilation_error.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/compi... pkg/front_end/lib/compilation_error.dart:17: abstract class CompilationError { On 2017/07/11 03:57:13, Siggi Cherem (dart-lang) wrote: > Paul - I like the idea of renaming this to CompilationMessage or something like > that. The reason: we will report other messages that are not errors (warnings, > hints, etc). > Fine by me :) https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/b... File pkg/front_end/lib/src/base/processed_options.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/b... pkg/front_end/lib/src/base/processed_options.dart:150: reportMessageNoLocation(messageCombinedCompileSdkAndSummary); IMO there should be a distinction between errors that are likely due to the user doing something wrong vs. errors that are almost certainly due to a bug in our code. The former kinds of errors make sense to report using our error reporting mechanism. For the latter, I think we should have a separate "internal error" reporting mechanism. For this case here (compileSdk is `true` and a summary was provided), the only way it could occur is due to a bug in the code invoking the front-end, so from the user's point of view it's an internal error. Having said that, it might perhaps make sense to have a single user-visible error code called "InternalError" (with room for a string containing details for the benefit of people developing the front end and its clients). *If* we decide to do that, then we could use that InternalError code here, with a hardcoded string detailing that the problem is that both compileSdk and a summary were specified. https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/fasta_codes.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/fasta_codes.dart:73: class LocatedMessage implements CompilationError { On 2017/07/11 03:57:13, Siggi Cherem (dart-lang) wrote: > Peter/Paul: a few questions for you here: > > 1- I would like to use messages.yaml for all of our errors, which means > practically that we'll use LocationMessage as the basic type we pass around in > the compiler and that eventually is used to produce errors publicly. > > To report an error we could either wrap these message objects or do what I did > in this patchset. Looking ahead, it seems to me like we'll want to wrap: the > public API needs to include the notion of severity (error/warning/hint/nit) and > I don't believe that should go here. Especially because different tools may want > to adjust the severity of messages (e.g. treat warnings as errors). > > What are your thoughts? do you agree with my thinking? If so, I'll update this > CL and wrap LocationMessage instead. Wrapping LocationMessage sounds reasonable to me. > > 2- How do we want to publicly expose analyzer and dart2js codes? Should we > simply return the String in [Code]? I'm not sure I understand the questino. Let's discuss in person. https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/kernel/verifier.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/kernel/verifier.dart:31: List<LocatedMessage> verifyProgram(Program program, {bool isOutline: false}) { I don't think it makes sense to use LocatedMessage here. Verification errors are internal errors and shouldn't use our user-visible error reporting mechanism. https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/test/kern... File pkg/front_end/test/kernel_generator_test.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/test/kern... pkg/front_end/test/kernel_generator_test.dart:59: expect(errors.first.message, contains("No 'main' method found")); Would it be possible to change these test assertions to refer to the generated code? If we did that, then later if we changed the error message text, we would not have to update the test. https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/tool/_fas... File pkg/front_end/tool/_fasta/generate_messages.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/tool/_fas... pkg/front_end/tool/_fasta/generate_messages.dart:56: return ''; Would it be better to throw an exception here, to make sure that the developer doesn't accidentally miss the warning message and assume compilation has succeeded?
https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/b... File pkg/front_end/lib/src/base/processed_options.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/b... pkg/front_end/lib/src/base/processed_options.dart:150: reportMessageNoLocation(messageCombinedCompileSdkAndSummary); On 2017/07/11 16:00:59, Paul Berry wrote: > IMO there should be a distinction between errors that are likely due to the user > doing something wrong vs. errors that are almost certainly due to a bug in our > code. The former kinds of errors make sense to report using our error reporting > mechanism. For the latter, I think we should have a separate "internal error" > reporting mechanism. > > For this case here (compileSdk is `true` and a summary was provided), the only > way it could occur is due to a bug in the code invoking the front-end, so from > the user's point of view it's an internal error. > > Having said that, it might perhaps make sense to have a single user-visible > error code called "InternalError" (with room for a string containing details for > the benefit of people developing the front end and its clients). *If* we decide > to do that, then we could use that InternalError code here, with a hardcoded > string detailing that the problem is that both compileSdk and a summary were > specified. Good points! I like however having precise messages when we know what the problem is. In particular, I'd like to get to the point where we have a searchable database for errors, even internal ones. How about we add a bool to [Code] that indicates if the error is internal?
On 2017/07/11 16:03:48, Siggi Cherem (dart-lang) wrote: > https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/b... > File pkg/front_end/lib/src/base/processed_options.dart (right): > > https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/b... > pkg/front_end/lib/src/base/processed_options.dart:150: > reportMessageNoLocation(messageCombinedCompileSdkAndSummary); > On 2017/07/11 16:00:59, Paul Berry wrote: > > IMO there should be a distinction between errors that are likely due to the > user > > doing something wrong vs. errors that are almost certainly due to a bug in our > > code. The former kinds of errors make sense to report using our error > reporting > > mechanism. For the latter, I think we should have a separate "internal error" > > reporting mechanism. > > > > For this case here (compileSdk is `true` and a summary was provided), the only > > way it could occur is due to a bug in the code invoking the front-end, so from > > the user's point of view it's an internal error. > > > > Having said that, it might perhaps make sense to have a single user-visible > > error code called "InternalError" (with room for a string containing details > for > > the benefit of people developing the front end and its clients). *If* we > decide > > to do that, then we could use that InternalError code here, with a hardcoded > > string detailing that the problem is that both compileSdk and a summary were > > specified. > > Good points! > > I like however having precise messages when we know what the problem is. In > particular, I'd like to get to the point where we have a searchable database for > errors, even internal ones. > > How about we add a bool to [Code] that indicates if the error is internal? I have some concerns we talked about in person. Just for the record: I want to make sure that internal errors don't accidentally become part of a public API. (E.g. it would be bad if we accidentally created a mechanism that allowed users to suppress an internal error by putting a comment in their source code naming the specific internal error, because that would mean that (a) users might suppress internal errors rather than reporting them, and (b) at a later date if we did an innocent refactor to the set of internal errors, it might break the user's code). Another concern is that I want to keep the barrier to writing defensive code low. E.g. if there's a condition that should never happen, I want someone to feel free to write something like: if (conditionThatShouldNeverHappen) { internalError('technobabble describing the condition'); } But if we standardize on reporting internal errors through the messages.yaml mechanism, that means that the same developer is going to have to update messages.yaml, come up with a thoughtful user-appropriate error message, renegenerate fasta_codes_generated.dart, and possibly other steps once we start publishing contextual information about each error on the web. I worry that's going to be a lot of extra work, so people won't be motivated to check for the condition in the first place.
Patchset #2 (id:40001) has been deleted
Thanks Paul for all the comments, I updated the CL to do the wrapping as we discussed. One remaining issue to decide: what do we want to do about internal errors (or non-user input errors). In processed-options I'm using the 'unsupported' template, since this is in a validate function that will let the caller gracefully handle the invalid input. For verification-errors, I'm not convinced on using intenralProblem becuase that API currently throws on the first error, and I'd prefer to collect all verification errors and report them. Passing all errors as regular errors like I do now is also not as attractive since we might want to fail compilation as soon as verification fails. Thoughts? https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/kernel/verifier.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/kernel/verifier.dart:31: List<LocatedMessage> verifyProgram(Program program, {bool isOutline: false}) { On 2017/07/11 16:00:59, Paul Berry wrote: > I don't think it makes sense to use LocatedMessage here. Verification errors > are internal errors and shouldn't use our user-visible error reporting > mechanism. I see your point and it is similar to the other error with compileSdk + summary in ProcessedOptions. Peter - what's your general opinion here? (for context, see also the discussion on the other parts of the CL) I was inclined to make all errors have an error code, unless the plan was to stop the compiler immediately (e.g. like throwing). For throwing-like errors, we can use problems.dart#unsupported, but if we can continue compilation after the error, I expect to append it to an error list and keep going. Verification errors might be the kind of error that should stop the compiler. https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/test/kern... File pkg/front_end/test/kernel_generator_test.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/test/kern... pkg/front_end/test/kernel_generator_test.dart:59: expect(errors.first.message, contains("No 'main' method found")); On 2017/07/11 16:00:59, Paul Berry wrote: > Would it be possible to change these test assertions to refer to the generated > code? If we did that, then later if we changed the error message text, we would > not have to update the test. In the future, yes, right now we don't expose the code in the public API. Added TODO https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/tool/_fas... File pkg/front_end/tool/_fasta/generate_messages.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/tool/_fas... pkg/front_end/tool/_fasta/generate_messages.dart:56: return ''; On 2017/07/11 16:00:59, Paul Berry wrote: > Would it be better to throw an exception here, to make sure that the developer > doesn't accidentally miss the warning message and assume compilation has > succeeded? Done.
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right): https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:100: final List<LocatedMessage> errors = <LocatedMessage>[]; note: this is now possible because I made deprecated_InputError implement LocatedMessage https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:337: new StringLiteral("${errors.map((e) => e.message).join('\n')}"))); this changes the generated code a bit, one of our tests shows the difference in the `.expect` files.
lgtm except for deprecated_InputError implementing LocatedMessage (see explanation below). We're both working on the same area right now, but there's very little conflict so far. Phew :-) https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/compi... File pkg/front_end/lib/compilation_error.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/compi... pkg/front_end/lib/compilation_error.dart:17: abstract class CompilationError { On 2017/07/11 03:57:13, Siggi Cherem (dart-lang) wrote: > Paul - I like the idea of renaming this to CompilationMessage or something like > that. The reason: we will report other messages that are not errors (warnings, > hints, etc). > CompilationProblem? https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/b... File pkg/front_end/lib/src/base/processed_options.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/b... pkg/front_end/lib/src/base/processed_options.dart:113: void deprecated_reportError(String error) { On 2017/07/11 03:57:13, Siggi Cherem (dart-lang) wrote: > FYI - I'm following Peter's style here, it is a transitional name to help us > migrate use sites. Thank you, this is helpful as I'm grepping for that prefix. https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/b... pkg/front_end/lib/src/base/processed_options.dart:121: void reportMessageNoLocation(Message message) => reportMessageNoLocation -> reportMessageWithoutLocation? https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/fasta_codes.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/fasta_codes.dart:73: class LocatedMessage implements CompilationError { On 2017/07/11 03:57:13, Siggi Cherem (dart-lang) wrote: > Peter/Paul: a few questions for you here: > > 1- I would like to use messages.yaml for all of our errors, which means > practically that we'll use LocationMessage as the basic type we pass around in > the compiler and that eventually is used to produce errors publicly. > > To report an error we could either wrap these message objects or do what I did > in this patchset. Looking ahead, it seems to me like we'll want to wrap: the > public API needs to include the notion of severity (error/warning/hint/nit) and > I don't believe that should go here. Especially because different tools may want > to adjust the severity of messages (e.g. treat warnings as errors). > > What are your thoughts? do you agree with my thinking? If so, I'll update this > CL and wrap LocationMessage instead. I agree. In fact, I just sent out CL 2974203002 which documents that messages should be able to change severity. Also, I have a strong preference for wrapping when it isn't performance critical. > 2- How do we want to publicly expose analyzer and dart2js codes? Should we > simply return the String in [Code]? I think we should just use a string (instead of an enum). A string is more flexible, and I think flexibility is a must when it comes to getting everyone on the same messages file. https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/deprecated_problems.dart (right): https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/deprecated_problems.dart:86: class deprecated_InputError implements LocatedMessage { I would like to maintain the invariant that instances of Message or LocatedMessage are derived from a message in messages.yaml. So it would be great if deprecated_InputError doesn't implement LocatedMessage. This is how I handle the similar situation in verifier.dart: problem(TreeNode node, String details, {TreeNode context}) { context ??= this.context; VerificationError error = new VerificationError(context, node, details); CompilerContext.current.report( templateUnspecified .withArguments("$error") .withLocation(Uri.parse(fileUri), node?.fileOffset ?? -1), Severity.error); errors.add(error); } CompilerContext.current.report is something that I've added based on what we discussed in CL 2979713002, and should make it easier to hook up the reporting you're adding with this CL. https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right): https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:100: final List<LocatedMessage> errors = <LocatedMessage>[]; On 2017/07/12 00:35:15, Siggi Cherem (dart-lang) wrote: > note: this is now possible because I made deprecated_InputError implement > LocatedMessage I'd like to hold back on this change. I'm pretty close to achieving the same effect, but without making deprecated_InputError implement LocatedMessage. https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:124: print(error.deprecated_format()); I'm pretty close to removing this and all the other calls to print from this class. https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:337: new StringLiteral("${errors.map((e) => e.message).join('\n')}"))); On 2017/07/12 00:35:15, Siggi Cherem (dart-lang) wrote: > this changes the generated code a bit, one of our tests shows the difference in > the `.expect` files. What's currently stored in the .expect files helps me catch problems when I break the compiler's output. One can argue that it isn't the purpose of the .expect files, but as I consider alternative, I'm actually starting to believe that having formatted messages in the .expect files is a convenient way to ensure that everything works as intended. https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/kernel/verifier.dart (right): https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/kernel/verifier.dart:111: LocatedMessage convertError(VerificationError error) { This is probably a better solution than what I have. https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/k... File pkg/front_end/lib/src/kernel_generator_impl.dart (right): https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/k... pkg/front_end/lib/src/kernel_generator_impl.dart:149: options.reportMessage(e); I suggest you use codeUnspecified here to convert e to a message. https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/test/kern... File pkg/front_end/test/kernel_generator_test.dart (right): https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/test/kern... pkg/front_end/test/kernel_generator_test.dart:61: expect(errors.first.message, contains("No 'main' method found")); Since this is a front_end test, you can import fasta_codes.dart and compare the codes now.
Sorry, I didn't see this discussion earlier. I've already landed some changes
that goes against your opinion. However, as I elaborate below, I hope you'll
find that what I've done doesn't create the problems you raise. And I'm
certainly willing to iterate more on this.
On 2017/07/11 17:00:31, Paul Berry wrote:
> I have some concerns we talked about in person. Just for the record: I want
to
> make sure that internal errors don't accidentally become part of a public API.
> (E.g. it would be bad if we accidentally created a mechanism that allowed
users
> to suppress an internal error by putting a comment in their source code naming
> the specific internal error, because that would mean that (a) users might
> suppress internal errors rather than reporting them, and (b) at a later date
if
> we did an innocent refactor to the set of internal errors, it might break the
> user's code).
I'm not sure I share that concern: an internal error will prevent us from
generating output.
I addition, if internal errors have their own severity level, we can just say
that analyisis_options.yaml doesn't apply.
>
> Another concern is that I want to keep the barrier to writing defensive code
> low. E.g. if there's a condition that should never happen, I want someone to
> feel free to write something like:
>
> if (conditionThatShouldNeverHappen) {
> internalError('technobabble describing the condition');
> }
>
> But if we standardize on reporting internal errors through the messages.yaml
> mechanism, that means that the same developer is going to have to update
> messages.yaml, come up with a thoughtful user-appropriate error message,
> renegenerate fasta_codes_generated.dart, and possibly other steps once we
start
> publishing contextual information about each error on the web. I worry that's
> going to be a lot of extra work, so people won't be motivated to check for the
> condition in the first place.
In practice, almost all internal errors fall in four categories:
1. Unhandled
2. Unexpected
3. Unsupported
4. Unimplemented
See
https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/src/fasta/prob...
You can almost always use unimplemented, but by insisting on a few defaults we
strongly encourage everyone from writing messages like "unreachable" or "can't
happen."
After completely removing internalError from Fasta, I was only added these
additional messages:
InternalProblemSuperclassNotFound:
template: "Superclass not found '#name'."
InternalProblemNotFound:
template: "Couldn't find '#name'."
InternalProblemNotFoundIn:
template: "Couldn't find '#name' in '#name2'."
InternalProblemPrivateConstructorAccess:
template: "Can't access private constructor '#name'."
InternalProblemConstructorNotFound:
template: "No constructor named '#name' in '#uri'."
InternalProblemExtendingUnmodifiableScope:
template: "Can't extend an unmodifiable scope."
InternalProblemPreviousTokenNotFound:
template: "Couldn't find previous token."
InternalProblemStackNotEmpty:
template: "#name.stack isn't empty:\n #string"
InternalProblemAlreadyInitialized:
template: "Attempt to set initializer on field without initializer."
InternalProblemBodyOnAbstractMethod:
template: "Attempting to set body on abstract method."
In each case, it was because I felt these internal errors are so important that
my co-workers were likely to see them frequently.
Patchset #3 (id:100001) has been deleted
addressed all comments. Made a minor change to `unimplemented` so it takes optional arguments (places where I used it do not have a location to report). Peter, PTAL at the last changes. Note: I tried rebasing on top of your CL and saw one small conflict because of the depracated_ rename, but otherwise no overlap between the two of us yet. For that reason I went ahead and landed this directly, feel free to revert and remerge if that's easier in your workflow. https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/compi... File pkg/front_end/lib/compilation_error.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/compi... pkg/front_end/lib/compilation_error.dart:17: abstract class CompilationError { On 2017/07/12 13:14:43, ahe wrote: > On 2017/07/11 03:57:13, Siggi Cherem (dart-lang) wrote: > > Paul - I like the idea of renaming this to CompilationMessage or something > like > > that. The reason: we will report other messages that are not errors (warnings, > > hints, etc). > > > > CompilationProblem? Is it always a problem? Don't we want the compiler sometimes to emit encouraging messages like "Good job, your code looks really neat!" ;-)? "problem" works for me too https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/b... File pkg/front_end/lib/src/base/processed_options.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/b... pkg/front_end/lib/src/base/processed_options.dart:113: void deprecated_reportError(String error) { On 2017/07/12 13:14:43, ahe wrote: > On 2017/07/11 03:57:13, Siggi Cherem (dart-lang) wrote: > > FYI - I'm following Peter's style here, it is a transitional name to help us > > migrate use sites. > > Thank you, this is helpful as I'm grepping for that prefix. https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/b... pkg/front_end/lib/src/base/processed_options.dart:121: void reportMessageNoLocation(Message message) => On 2017/07/12 13:14:43, ahe wrote: > reportMessageNoLocation -> reportMessageWithoutLocation? Done. https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/kernel/verifier.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/kernel/verifier.dart:31: List<LocatedMessage> verifyProgram(Program program, {bool isOutline: false}) { On 2017/07/12 00:29:46, Siggi Cherem (dart-lang) wrote: > On 2017/07/11 16:00:59, Paul Berry wrote: > > I don't think it makes sense to use LocatedMessage here. Verification errors > > are internal errors and shouldn't use our user-visible error reporting > > mechanism. > > I see your point and it is similar to the other error with compileSdk + summary > in ProcessedOptions. > > Peter - what's your general opinion here? (for context, see also the discussion > on the other parts of the CL) > > I was inclined to make all errors have an error code, unless the plan was to > stop the compiler immediately (e.g. like throwing). For throwing-like errors, we > can use problems.dart#unsupported, but if we can continue compilation after the > error, I expect to append it to an error list and keep going. > > Verification errors might be the kind of error that should stop the compiler. Just to close the loop here - we started using "InternalProblem" prefix for all kind of internal errors, so we can easily distinguish them and treat them in a special way on our tools. We might want to add more in the API to expose when a message is from an internal problem (other than the code name), though. Let's keep iterating on this. https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/deprecated_problems.dart (right): https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/deprecated_problems.dart:86: class deprecated_InputError implements LocatedMessage { On 2017/07/12 13:14:43, ahe wrote: > I would like to maintain the invariant that instances of Message or > LocatedMessage are derived from a message in messages.yaml. So it would be great > if deprecated_InputError doesn't implement LocatedMessage. > > This is how I handle the similar situation in verifier.dart: > > problem(TreeNode node, String details, {TreeNode context}) { > context ??= this.context; > VerificationError error = new VerificationError(context, node, details); > CompilerContext.current.report( > templateUnspecified > .withArguments("$error") > .withLocation(Uri.parse(fileUri), node?.fileOffset ?? -1), > Severity.error); > errors.add(error); > } > > CompilerContext.current.report is something that I've added based on what we > discussed in CL 2979713002, and should make it easier to hook up the reporting > you're adding with this CL. Done (removed the implements here) https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right): https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:100: final List<LocatedMessage> errors = <LocatedMessage>[]; On 2017/07/12 13:14:43, ahe wrote: > On 2017/07/12 00:35:15, Siggi Cherem (dart-lang) wrote: > > note: this is now possible because I made deprecated_InputError implement > > LocatedMessage > > I'd like to hold back on this change. I'm pretty close to achieving the same > effect, but without making deprecated_InputError implement LocatedMessage. OK, I reverted this. https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:124: print(error.deprecated_format()); On 2017/07/12 13:14:43, ahe wrote: > I'm pretty close to removing this and all the other calls to print from this > class. fantastic!, reverted to prevent conflicts. https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:337: new StringLiteral("${errors.map((e) => e.message).join('\n')}"))); On 2017/07/12 13:14:43, ahe wrote: > On 2017/07/12 00:35:15, Siggi Cherem (dart-lang) wrote: > > this changes the generated code a bit, one of our tests shows the difference > in > > the `.expect` files. > > What's currently stored in the .expect files helps me catch problems when I > break the compiler's output. One can argue that it isn't the purpose of the > .expect files, but as I consider alternative, I'm actually starting to believe > that having formatted messages in the .expect files is a convenient way to > ensure that everything works as intended. reverted this for now as well https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/k... File pkg/front_end/lib/src/kernel_generator_impl.dart (right): https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/lib/src/k... pkg/front_end/lib/src/kernel_generator_impl.dart:149: options.reportMessage(e); On 2017/07/12 13:14:43, ahe wrote: > I suggest you use codeUnspecified here to convert e to a message. Makes sense, but I reverted this for now, since I think you have a handle on the InputErrors on your end. https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/test/kern... File pkg/front_end/test/kernel_generator_test.dart (right): https://codereview.chromium.org/2979623002/diff/80001/pkg/front_end/test/kern... pkg/front_end/test/kernel_generator_test.dart:61: expect(errors.first.message, contains("No 'main' method found")); On 2017/07/12 13:14:44, ahe wrote: > Since this is a front_end test, you can import fasta_codes.dart and compare the > codes now. Not quite: we are not exposing the `Code` class from the public API. We should expose something in the future, but we can deal with that separately.
Description was changed from ========== Use messages for (some) public API errors not entirely ready - looking for comments. BUG= ========== to ========== Use messages for (some) public API errors not entirely ready - looking for comments. BUG= R=ahe@google.com Committed: https://github.com/dart-lang/sdk/commit/a7865761d4829bc5d3cbd7da857faba0081311b4 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001) manually as a7865761d4829bc5d3cbd7da857faba0081311b4 (presubmit successful).
Message was sent while issue was closed.
On 2017/07/12 21:55:00, Siggi Cherem (dart-lang) wrote: > addressed all comments. Made a minor change to `unimplemented` so it takes > optional arguments (places where I used it do not have a location to report). I specifically made these methods require arguments to prevent a common mistake: forgetting to pass in a location when you have it handy. Could we make them mandatory again? > Peter, PTAL at the last changes. > > Note: I tried rebasing on top of your CL and saw one small conflict because of > the depracated_ rename, but otherwise no overlap between the two of us yet. For > that reason I went ahead and landed this directly, feel free to revert and > remerge if that's easier in your workflow. I have already rebased on your changes. No problem.
Message was sent while issue was closed.
https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/compi... File pkg/front_end/lib/compilation_error.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/compi... pkg/front_end/lib/compilation_error.dart:17: abstract class CompilationError { On 2017/07/12 21:54:58, Siggi Cherem (dart-lang) wrote: > On 2017/07/12 13:14:43, ahe wrote: > > On 2017/07/11 03:57:13, Siggi Cherem (dart-lang) wrote: > > > Paul - I like the idea of renaming this to CompilationMessage or something > > like > > > that. The reason: we will report other messages that are not errors > (warnings, > > > hints, etc). > > > > > > > CompilationProblem? > > Is it always a problem? Don't we want the compiler sometimes to emit encouraging > messages like "Good job, your code looks really neat!" ;-)? > > "problem" works for me too CompilationOpportunity? ;-)
Message was sent while issue was closed.
On 2017/07/12 23:37:34, ahe wrote: > On 2017/07/12 21:55:00, Siggi Cherem (dart-lang) wrote: > > addressed all comments. Made a minor change to `unimplemented` so it takes > > optional arguments (places where I used it do not have a location to report). > > I specifically made these methods require arguments to prevent a common mistake: > forgetting to pass in a location when you have it handy. Could we make them > mandatory again? > sure thing > > > Peter, PTAL at the last changes. > > > > Note: I tried rebasing on top of your CL and saw one small conflict because of > > the depracated_ rename, but otherwise no overlap between the two of us yet. > For > > that reason I went ahead and landed this directly, feel free to revert and > > remerge if that's easier in your workflow. > > I have already rebased on your changes. No problem.
Message was sent while issue was closed.
Still lgtm, but please reconsider making the position parameters mandatory on unimplemented. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/lib/src/... File pkg/front_end/lib/src/base/processed_options.dart (right): https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/lib/src/... pkg/front_end/lib/src/base/processed_options.dart:122: _raw.onError(new _CompilationMessage(message)); We need to think about how these methods deal with _raw.onError throwing an exception. We do not want to report that as a crash if we add crash reporting like in dart2js. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/lib/src/... pkg/front_end/lib/src/base/processed_options.dart:135: templateMissingInputFile.withArguments('$source')); There's a #uri parameter you can use now. It will automatically make the URI relative, but preserve the full URI in the arguments object. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/lib/src/... pkg/front_end/lib/src/base/processed_options.dart:143: templateMissingSdkRoot.withArguments('$sdkRoot')); Ditto. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/lib/src/... pkg/front_end/lib/src/base/processed_options.dart:150: templateMissingSdkSummary.withArguments('$summary')); Ditto. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/lib/src/... pkg/front_end/lib/src/base/processed_options.dart:157: "The compileSdk and sdkSummary options are mutually exclusive")); I think this should be its own error code, but I can live with this. I think so because this error is likely to be seen by users of the API. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/lib/src/... pkg/front_end/lib/src/base/processed_options.dart:363: final LocatedMessage original; This should probably be private as you've stated several times that properties of this object aren't exposed in the public API. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/lib/src/... pkg/front_end/lib/src/base/processed_options.dart:378: /// Wraps an error message as a [CompilationError] API. Use templateUnspecified instead? https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/messages... File pkg/front_end/messages.yaml (right): https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/messages... pkg/front_end/messages.yaml:652: template: "Input file not found: #string." #uri here and below? https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/test/ker... File pkg/front_end/test/kernel_generator_test.dart (right): https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/test/ker... pkg/front_end/test/kernel_generator_test.dart:61: expect(errors.first.message, contains("No 'main' method found")); I think this might work: equals(messageMissingMain.message) https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/test/src... File pkg/front_end/test/src/base/processed_options_test.dart (right): https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/test/src... pkg/front_end/test/src/base/processed_options_test.dart:165: expect(errors.first.message, contains("SDK root directory not found")); For the same trick to work here, you'd have to remove the #string (or #uri) parameter. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/tool/_fa... File pkg/front_end/tool/_fasta/generate_messages.dart (right): https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/tool/_fa... pkg/front_end/tool/_fasta/generate_messages.dart:58: exit(1); How about: exitCode = 1;
Message was sent while issue was closed.
https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/compi... File pkg/front_end/lib/compilation_error.dart (right): https://codereview.chromium.org/2979623002/diff/20001/pkg/front_end/lib/compi... pkg/front_end/lib/compilation_error.dart:17: abstract class CompilationError { On 2017/07/12 23:44:56, ahe wrote: > On 2017/07/12 21:54:58, Siggi Cherem (dart-lang) wrote: > > On 2017/07/12 13:14:43, ahe wrote: > > > On 2017/07/11 03:57:13, Siggi Cherem (dart-lang) wrote: > > > > Paul - I like the idea of renaming this to CompilationMessage or something > > > like > > > > that. The reason: we will report other messages that are not errors > > (warnings, > > > > hints, etc). > > > > > > > > > > CompilationProblem? > > > > Is it always a problem? Don't we want the compiler sometimes to emit > encouraging > > messages like "Good job, your code looks really neat!" ;-)? > > > > "problem" works for me too > > CompilationOpportunity? ;-) Comportunity? ;-)
Message was sent while issue was closed.
https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/lib/src/... File pkg/front_end/lib/src/base/processed_options.dart (right): https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/lib/src/... pkg/front_end/lib/src/base/processed_options.dart:122: _raw.onError(new _CompilationMessage(message)); On 2017/07/13 00:03:38, ahe wrote: > We need to think about how these methods deal with _raw.onError throwing an > exception. > > We do not want to report that as a crash if we add crash reporting like in > dart2js. Acknowledged. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/lib/src/... pkg/front_end/lib/src/base/processed_options.dart:135: templateMissingInputFile.withArguments('$source')); On 2017/07/13 00:03:38, ahe wrote: > There's a #uri parameter you can use now. It will automatically make the URI > relative, but preserve the full URI in the arguments object. Done. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/lib/src/... pkg/front_end/lib/src/base/processed_options.dart:143: templateMissingSdkRoot.withArguments('$sdkRoot')); On 2017/07/13 00:03:38, ahe wrote: > Ditto. Done. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/lib/src/... pkg/front_end/lib/src/base/processed_options.dart:150: templateMissingSdkSummary.withArguments('$summary')); On 2017/07/13 00:03:38, ahe wrote: > Ditto. Done. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/lib/src/... pkg/front_end/lib/src/base/processed_options.dart:157: "The compileSdk and sdkSummary options are mutually exclusive")); On 2017/07/13 00:03:37, ahe wrote: > I think this should be its own error code, but I can live with this. > > I think so because this error is likely to be seen by users of the API. Done. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/lib/src/... pkg/front_end/lib/src/base/processed_options.dart:363: final LocatedMessage original; On 2017/07/13 00:03:38, ahe wrote: > This should probably be private as you've stated several times that properties > of this object aren't exposed in the public API. Ok, I've added this to the CL where I'm unifying CompilerContext and ProcessedOptions. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/lib/src/... pkg/front_end/lib/src/base/processed_options.dart:378: /// Wraps an error message as a [CompilationError] API. On 2017/07/13 00:03:38, ahe wrote: > Use templateUnspecified instead? This is now gone. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/messages... File pkg/front_end/messages.yaml (right): https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/messages... pkg/front_end/messages.yaml:652: template: "Input file not found: #string." On 2017/07/13 00:03:38, ahe wrote: > #uri here and below? Done. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/test/ker... File pkg/front_end/test/kernel_generator_test.dart (right): https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/test/ker... pkg/front_end/test/kernel_generator_test.dart:61: expect(errors.first.message, contains("No 'main' method found")); On 2017/07/13 00:03:38, ahe wrote: > I think this might work: > > equals(messageMissingMain.message) Done. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/test/src... File pkg/front_end/test/src/base/processed_options_test.dart (right): https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/test/src... pkg/front_end/test/src/base/processed_options_test.dart:165: expect(errors.first.message, contains("SDK root directory not found")); On 2017/07/13 00:03:38, ahe wrote: > For the same trick to work here, you'd have to remove the #string (or #uri) > parameter. Done. Already landed this fix in a separate CL. https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/tool/_fa... File pkg/front_end/tool/_fasta/generate_messages.dart (right): https://codereview.chromium.org/2979623002/diff/120001/pkg/front_end/tool/_fa... pkg/front_end/tool/_fasta/generate_messages.dart:58: exit(1); On 2017/07/13 00:03:38, ahe wrote: > How about: > > exitCode = 1; Done. |
