Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1497253002:
Align strong-mode error codes with analyzer conv. (Closed)
|
Created:
5 years ago by pquitslund Modified:
5 years ago CC:
reviews_dartlang.org, Leaf, scheglov Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAlign strong-mode error codes with analyzer conv.
This canonicalization will allow suppression by code name to "just work" unblocking https://github.com/flutter/flutter/issues/624.
R=jmesserly@google.com, leafp@google.com
Committed: https://github.com/dart-lang/sdk/commit/eacbee4616b7d06c42966240d7bb143cf07908fa
Patch Set 1 #
Total comments: 8
Patch Set 2 : test_fixes #Patch Set 3 : doc_fixes #
Total comments: 30
Patch Set 4 : doc_fix #
Total comments: 1
Patch Set 5 : doc_fix_cont #
Messages
Total messages: 17 (3 generated)
pquitslund@google.com changed reviewers: + jmesserly@google.com
+Leaf as FYI, LGTM though. BTW, if we wanted, we could keep the test cases the same by converting from UpperCamelCase to SCREAMING_CAPS on the fly (we have to parse those comments anyway in the test code). But doesn't seem worth it.
On 2015/12/04 18:19:52, John Messerly wrote: > +Leaf as FYI, LGTM though. Thanks! > > BTW, if we wanted, we could keep the test cases the same by converting from > UpperCamelCase to SCREAMING_CAPS on the fly (we have to parse those comments > anyway in the test code). But doesn't seem worth it. I thought about that. I actually thought about being clever and extracting the names from their runtimeTypes as well but decided to just stay simple for now.
On 2015/12/04 18:22:08, pquitslund wrote: > On 2015/12/04 18:19:52, John Messerly wrote: > > +Leaf as FYI, LGTM though. > > Thanks! > > > > > BTW, if we wanted, we could keep the test cases the same by converting from > > UpperCamelCase to SCREAMING_CAPS on the fly (we have to parse those comments > > anyway in the test code). But doesn't seem worth it. > > I thought about that. I actually thought about being clever and extracting the > names from their runtimeTypes as well but decided to just stay simple for now. Aside: another thing to discuss along the lines of alignment is that in analyzer we share single instances of error codes. Not sure if that's a feasible approach given how these are factored though it's worth pointing out that if we can't I'll need to create sidecar datastructure to keep track of valid strong-mode error code names (since I can't put them in the ErrorCode values "enum" --- see ErrorCode.values). Not a big deal frankly, but I'll want to do it in order to not see false negatives when validating .analysis_options files that suppress strong-mode warnings. Here's a bug tracking that: https://github.com/dart-lang/sdk/issues/25117
leafp@google.com changed reviewers: + leafp@google.com
LGTM modulo fixing whatever happened with the comments in info.dart. https://codereview.chromium.org/1497253002/diff/1/pkg/analyzer/lib/src/task/s... File pkg/analyzer/lib/src/task/strong/info.dart (right): https://codereview.chromium.org/1497253002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong/info.dart:19: // check for errors and reduce redundant coercions to the identity. I think something went very wrong with the sorting of members in this file. All of the comments for the classes stayed behind and are now out of place. Possibly partially our fault for not using doc comments? Not sure all comments outside of classes should be doc comments though, so not sure what the right answer is. https://codereview.chromium.org/1497253002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong/info.dart:35: // The identity coercion Comment out of place. https://codereview.chromium.org/1497253002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong/info.dart:55: // argument. ditto. https://codereview.chromium.org/1497253002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong/info.dart:179: // Base class for all casts from base type to sub type. I don't understand how this comment got here at all, but presumably an artifact of the sorting somehow?
On 2015/12/04 19:01:11, Leaf wrote: > LGTM modulo fixing whatever happened with the comments in info.dart. Thanks! > > https://codereview.chromium.org/1497253002/diff/1/pkg/analyzer/lib/src/task/s... > File pkg/analyzer/lib/src/task/strong/info.dart (right): > > https://codereview.chromium.org/1497253002/diff/1/pkg/analyzer/lib/src/task/s... > pkg/analyzer/lib/src/task/strong/info.dart:19: // check for errors and reduce > redundant coercions to the identity. > I think something went very wrong with the sorting of members in this file. All > of the comments for the classes stayed behind and are now out of place. > Possibly partially our fault for not using doc comments? Not sure all comments > outside of classes should be doc comments though, so not sure what the right > answer is. :( That's a bummer for sure. I'll fix these in the short term. Down the road we can do a bit better with our sorter. > > https://codereview.chromium.org/1497253002/diff/1/pkg/analyzer/lib/src/task/s... > pkg/analyzer/lib/src/task/strong/info.dart:35: // The identity coercion > Comment out of place. > > https://codereview.chromium.org/1497253002/diff/1/pkg/analyzer/lib/src/task/s... > pkg/analyzer/lib/src/task/strong/info.dart:55: // argument. > ditto. > > https://codereview.chromium.org/1497253002/diff/1/pkg/analyzer/lib/src/task/s... > pkg/analyzer/lib/src/task/strong/info.dart:179: // Base class for all casts from > base type to sub type. > I don't understand how this comment got here at all, but presumably an artifact > of the sorting somehow? Yep. It's wild west with non-doc-comments.
Thanks! (And sorry for all the noise :)) https://codereview.chromium.org/1497253002/diff/1/pkg/analyzer/lib/src/task/s... File pkg/analyzer/lib/src/task/strong/info.dart (right): https://codereview.chromium.org/1497253002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong/info.dart:19: // check for errors and reduce redundant coercions to the identity. On 2015/12/04 19:01:11, Leaf wrote: > I think something went very wrong with the sorting of members in this file. All > of the comments for the classes stayed behind and are now out of place. > Possibly partially our fault for not using doc comments? Not sure all comments > outside of classes should be doc comments though, so not sure what the right > answer is. Yep. In this case, I think doc comments make sense but in general we need to fix the sorter to be smarter. https://codereview.chromium.org/1497253002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong/info.dart:35: // The identity coercion On 2015/12/04 19:01:11, Leaf wrote: > Comment out of place. Done. https://codereview.chromium.org/1497253002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong/info.dart:55: // argument. On 2015/12/04 19:01:11, Leaf wrote: > ditto. Done. https://codereview.chromium.org/1497253002/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong/info.dart:179: // Base class for all casts from base type to sub type. On 2015/12/04 19:01:11, Leaf wrote: > I don't understand how this comment got here at all, but presumably an artifact > of the sorting somehow? Done.
Still a bunch of stuff in the wrong place, I commented on a bunch. https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... File pkg/analyzer/lib/src/task/strong/info.dart (right): https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:17: // The abstract type of coercions mapping one type to another. I think this comment got duped from below? https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:179: // Implicit down casts. These are only injected by the compiler by flag. This comment goes with DownCastImplicit. https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:195: // Standard down casts. These casts are implicitly injected by the compiler. This comment goes with DownCast https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:209: // A down cast due to a variable declaration to a ground type. E.g., This comment goes with AssignmentCast https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:224: // Temporary "casts" of allocation sites - literals, constructor invocations, This cast goes with UninferredClosure https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:266: // A down cast to a non-ground type. These behave differently from standard DownCastImplicit https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:272: // Factory to create correct InferredType variant. This goes with the create Factory https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:293: // reified into the term InferredTypeBase https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:302: // Standard / unspecialized inferred type InferredType. https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:317: // An inferred type for a literal expression. InferredTypeLiteral https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:326: // An inferred type for a non-literal allocation site. InferredTypeAllocation https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:335: // An inferred type for a closure expression InferredTypeClosure https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:348: // Invalid override due to incompatible type. I.e., the overridden signature InvalidFieldOverride https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:486: // Invalid override of an instance member of a class. InvalidOverride https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:505: // as well as some DDC specific [ErrorType]s. toErrorCode
Would that I could go back in time to before I clicked "Sort Members"! ;) Should be good now. Sorry for all the noise! @scheglov FYI in case it's easy to make "Sort Members" play nicer with non-doc comments. Aside: is there a good reason that these are not doc comments? I'm not sure I understand the rationale for why they're not (besides TODOs). https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... File pkg/analyzer/lib/src/task/strong/info.dart (right): https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:17: // The abstract type of coercions mapping one type to another. On 2015/12/04 19:32:15, Leaf wrote: > I think this comment got duped from below? Done. https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:179: // Implicit down casts. These are only injected by the compiler by flag. On 2015/12/04 19:32:14, Leaf wrote: > This comment goes with DownCastImplicit. Done. https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:195: // Standard down casts. These casts are implicitly injected by the compiler. On 2015/12/04 19:32:15, Leaf wrote: > This comment goes with DownCast `DynamicCast` I think? https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:209: // A down cast due to a variable declaration to a ground type. E.g., On 2015/12/04 19:32:15, Leaf wrote: > This comment goes with AssignmentCast Done. https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:224: // Temporary "casts" of allocation sites - literals, constructor invocations, On 2015/12/04 19:32:15, Leaf wrote: > This cast goes with UninferredClosure Done. https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:266: // A down cast to a non-ground type. These behave differently from standard On 2015/12/04 19:32:15, Leaf wrote: > DownCastImplicit Done. https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:272: // Factory to create correct InferredType variant. On 2015/12/04 19:32:14, Leaf wrote: > This goes with the create Factory Done. https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:293: // reified into the term On 2015/12/04 19:32:14, Leaf wrote: > InferredTypeBase Done. https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:302: // Standard / unspecialized inferred type On 2015/12/04 19:32:14, Leaf wrote: > InferredType. Done. https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:317: // An inferred type for a literal expression. On 2015/12/04 19:32:14, Leaf wrote: > InferredTypeLiteral Done. https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:326: // An inferred type for a non-literal allocation site. On 2015/12/04 19:32:14, Leaf wrote: > InferredTypeAllocation Done. https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:335: // An inferred type for a closure expression On 2015/12/04 19:32:15, Leaf wrote: > InferredTypeClosure Done. https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:348: // Invalid override due to incompatible type. I.e., the overridden signature On 2015/12/04 19:32:14, Leaf wrote: > InvalidFieldOverride Done. https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:486: // Invalid override of an instance member of a class. On 2015/12/04 19:32:15, Leaf wrote: > InvalidOverride Done. https://codereview.chromium.org/1497253002/diff/40001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:505: // as well as some DDC specific [ErrorType]s. On 2015/12/04 19:32:14, Leaf wrote: > toErrorCode Done.
Description was changed from ========== Align strong-mode error codes with analyzer conv. This canonicalization will allow suppression by code name to "just work" unblocking https://github.com/flutter/flutter/issues/624. ========== to ========== Align strong-mode error codes with analyzer conv. This canonicalization will allow suppression by code name to "just work" unblocking https://github.com/flutter/flutter/issues/624. R=jmesserly@google.com, leafp@google.com Committed: https://github.com/dart-lang/sdk/commit/eacbee4616b7d06c42966240d7bb143cf07908fa ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as eacbee4616b7d06c42966240d7bb143cf07908fa (presubmit successful).
Message was sent while issue was closed.
There's a comment that should probably be deleted now, but basically lgtm. > Aside: is there a good reason that these are not doc comments? I'm not sure I > understand the rationale for why they're not (besides TODOs). I can't speak for anyone else, but for myself, the main "rationale" is simply that I've done most of my programming in languages without a convention of doc comments separate from regular comments, so I'm really just getting used to using them, and don't yet have a good mental model of when to use them and when not. :) So I'd be perfectly happy making those doc comments. That aside, a couple of things I would say: 1) That code was not written to be sorted. If you look at the comment on line 238 of the original file, that comment is delineating a section of the file which contains related types. I actually vastly prefer this for many kinds of programming to the "sort the world" approach. It's ok, I'm fine adapting to whatever style is standard for a language/repository, but as originally written, that comment was going to be out of place no matter what happened in sorting. 2) Not all top level members of a file are intended to be part of the API. I don't know how the doc tool works - does it just export every top level declaration as part of the API? If so, why not just treat every top level comment as a doc comment as well? Basically, my mental model has been that doc comments should be only on things that are part of the API, and only for comments that are useful to consumers of the API. But perhaps that model needs work... :) 3) TODOS and company... :) https://codereview.chromium.org/1497253002/diff/60001/pkg/analyzer/lib/src/ta... File pkg/analyzer/lib/src/task/strong/info.dart (right): https://codereview.chromium.org/1497253002/diff/60001/pkg/analyzer/lib/src/ta... pkg/analyzer/lib/src/task/strong/info.dart:207: // Standard down casts. These casts are implicitly injected by the compiler. this comment should go away, it only makes sense if the declarations are grouped.
Message was sent while issue was closed.
On 2015/12/04 23:58:43, Leaf wrote: > There's a comment that should probably be deleted now, but basically lgtm. > > > Aside: is there a good reason that these are not doc comments? I'm not sure I > > understand the rationale for why they're not (besides TODOs). > > I can't speak for anyone else, but for myself, the main "rationale" is simply > that I've done most of my programming in languages without a convention of doc > comments separate from regular comments, so I'm really just getting used to > using them, and don't yet have a good mental model of when to use them and when > not. :) So I'd be perfectly happy making those doc comments. That aside, a > couple of things I would say: > > 1) That code was not written to be sorted. If you look at the comment on line > 238 of the original file, that comment is delineating a section of the file > which contains related types. I actually vastly prefer this for many kinds of > programming to the "sort the world" approach. It's ok, I'm fine adapting to > whatever style is standard for a language/repository, but as originally written, > that comment was going to be out of place no matter what happened in sorting. > > 2) Not all top level members of a file are intended to be part of the API. I > don't know how the doc tool works - does it just export every top level > declaration as part of the API? If so, why not just treat every top level > comment as a doc comment as well? Basically, my mental model has been that doc > comments should be only on things that are part of the API, and only for > comments that are useful to consumers of the API. But perhaps that model needs > work... :) > > > 3) TODOS and company... :) > > https://codereview.chromium.org/1497253002/diff/60001/pkg/analyzer/lib/src/ta... > File pkg/analyzer/lib/src/task/strong/info.dart (right): > > https://codereview.chromium.org/1497253002/diff/60001/pkg/analyzer/lib/src/ta... > pkg/analyzer/lib/src/task/strong/info.dart:207: // Standard down casts. These > casts are implicitly injected by the compiler. > this comment should go away, it only makes sense if the declarations are > grouped. At some point, we should really raise the issue of sorting. It causes me a *lot* of pain when working on code, reading diffs, merging, chasing down related helper methods that are 10000 lines down the file. It seems to have low value and very real cost associated with it.
Message was sent while issue was closed.
On 2015/12/04 23:58:43, Leaf wrote: > There's a comment that should probably be deleted now, but basically lgtm. Cool! > > > Aside: is there a good reason that these are not doc comments? I'm not sure I > > understand the rationale for why they're not (besides TODOs). > > I can't speak for anyone else, but for myself, the main "rationale" is simply > that I've done most of my programming in languages without a convention of doc > comments separate from regular comments, so I'm really just getting used to > using them, and don't yet have a good mental model of when to use them and when > not. :) So I'd be perfectly happy making those doc comments. That aside, a > couple of things I would say: > > 1) That code was not written to be sorted. If you look at the comment on line > 238 of the original file, that comment is delineating a section of the file > which contains related types. I actually vastly prefer this for many kinds of > programming to the "sort the world" approach. It's ok, I'm fine adapting to > whatever style is standard for a language/repository, but as originally written, > that comment was going to be out of place no matter what happened in sorting. +1. I'm not hugely in favor of the sorting convention but I've gotten used to it and "when in Rome..." > > 2) Not all top level members of a file are intended to be part of the API. I > don't know how the doc tool works - does it just export every top level > declaration as part of the API? If so, why not just treat every top level > comment as a doc comment as well? Basically, my mental model has been that doc > comments should be only on things that are part of the API, and only for > comments that are useful to consumers of the API. But perhaps that model needs > work... :) This merits a longer discussion. Ideally with beer! :) > > > 3) TODOS and company... :) > > https://codereview.chromium.org/1497253002/diff/60001/pkg/analyzer/lib/src/ta... > File pkg/analyzer/lib/src/task/strong/info.dart (right): > > https://codereview.chromium.org/1497253002/diff/60001/pkg/analyzer/lib/src/ta... > pkg/analyzer/lib/src/task/strong/info.dart:207: // Standard down casts. These > casts are implicitly injected by the compiler. > this comment should go away, it only makes sense if the declarations are > grouped. Ah! Right. Fixed here: https://codereview.chromium.org/1501573004/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
