Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1367183004:
analyzer strong mode: infer final field from initializer if super getters are dynamic (Closed)
|
Created:
5 years, 2 months ago by Jennifer Messerly Modified:
5 years, 2 months ago CC:
reviews_dartlang.org Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
Descriptionanalyzer strong mode: infer final field from initializer if super getters are dynamic
fixes https://github.com/dart-lang/dev_compiler/issues/348
R=brianwilkerson@google.com, leafp@google.com
Committed: https://github.com/dart-lang/sdk/commit/7bd3db6d741088705063373cec6b8d099225cd1c
Patch Set 1 #
Total comments: 7
Patch Set 2 : #Patch Set 3 : fix comment #Patch Set 4 : redo comment per Brian's suggestion #
Messages
Total messages: 10 (1 generated)
jmesserly@google.com changed reviewers: + brianwilkerson@google.com, leafp@google.com, vsm@google.com
This definitely fixes the issue, but I'm not 100% sure about it. Thoughts appreciated. https://codereview.chromium.org/1367183004/diff/1/pkg/analyzer/lib/src/task/s... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1367183004/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:304: if (newType == null || newType.isDynamic) { this fix is intended to match: https://github.com/dart-lang/dev_compiler/blob/1549e6a5e7e07a7f0ccb630dbaa926... To be honest, I'm not sure why we special case `dynamic` ... instead of either using hasImplicitType, or just a subtype check? Thoughts?
LGTM https://codereview.chromium.org/1367183004/diff/1/pkg/analyzer/lib/src/task/s... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1367183004/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:304: if (newType == null || newType.isDynamic) { > To be honest, I'm not sure why we special case `dynamic` ... instead of either > using hasImplicitType, or just a subtype check? Thoughts? hasImplicitType only applies to variables, not types. What we're saying here is that we will try to infer the type from the initializer if either there was no overridden getter or if the overridden getter couldn't have its type inferred. (But the comment you added makes no sense here, so please remove it.)
Thanks! Question on your comment. https://codereview.chromium.org/1367183004/diff/1/pkg/analyzer/lib/src/task/s... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1367183004/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:304: if (newType == null || newType.isDynamic) { On 2015/09/25 22:00:00, Brian Wilkerson wrote: > > To be honest, I'm not sure why we special case `dynamic` ... instead of either > > using hasImplicitType, or just a subtype check? Thoughts? > > hasImplicitType only applies to variables, not types. > > What we're saying here is that we will try to infer the type from the > initializer if either there was no overridden getter or if the overridden getter > couldn't have its type inferred. > > (But the comment you added makes no sense here, so please remove it.) What doesn't make sense about the comment? That's basically Siggi's comment from DDC's old resolver code ... it made sense to me. Let me try and reword ... he was saying: because the supertype field's getter is `dynamic`, it is safe to infer a stronger type in a derived class. This won't break any users of the supertype, because they already have to expect it to be any possible type. (and we get `newType` from `newType = _computeReturnType(overriddenGetters);` above. In other words, it represents the return type of the overridden getters) I'm happy to reword, but I don't want to just remove the comment...
lgtm https://codereview.chromium.org/1367183004/diff/1/pkg/analyzer/lib/src/task/s... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1367183004/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:304: if (newType == null || newType.isDynamic) { On 2015/09/25 22:22:11, John Messerly wrote: > On 2015/09/25 22:00:00, Brian Wilkerson wrote: > > > To be honest, I'm not sure why we special case `dynamic` ... instead of > either > > > using hasImplicitType, or just a subtype check? Thoughts? > > > > hasImplicitType only applies to variables, not types. > > > > What we're saying here is that we will try to infer the type from the > > initializer if either there was no overridden getter or if the overridden > getter > > couldn't have its type inferred. > > > > (But the comment you added makes no sense here, so please remove it.) > > What doesn't make sense about the comment? That's basically Siggi's comment from > DDC's old resolver code ... it made sense to me. Let me try and reword ... he > was saying: because the supertype field's getter is `dynamic`, it is safe to > infer a stronger type in a derived class. This won't break any users of the > supertype, because they already have to expect it to be any possible type. > > (and we get `newType` from `newType = _computeReturnType(overriddenGetters);` > above. In other words, it represents the return type of the overridden getters) > > I'm happy to reword, but I don't want to just remove the comment... The comment has a wording problem in the first line ("if we're the field is final"), but the gist of it makes sense to me. I'm also not sure why we only handle the dynamic case.
https://codereview.chromium.org/1367183004/diff/1/pkg/analyzer/lib/src/task/s... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1367183004/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:304: if (newType == null || newType.isDynamic) { On 2015/09/25 22:36:32, Leaf wrote: > On 2015/09/25 22:22:11, John Messerly wrote: > > On 2015/09/25 22:00:00, Brian Wilkerson wrote: > > > > To be honest, I'm not sure why we special case `dynamic` ... instead of > > either > > > > using hasImplicitType, or just a subtype check? Thoughts? > > > > > > hasImplicitType only applies to variables, not types. > > > > > > What we're saying here is that we will try to infer the type from the > > > initializer if either there was no overridden getter or if the overridden > > getter > > > couldn't have its type inferred. > > > > > > (But the comment you added makes no sense here, so please remove it.) > > > > What doesn't make sense about the comment? That's basically Siggi's comment > from > > DDC's old resolver code ... it made sense to me. Let me try and reword ... he > > was saying: because the supertype field's getter is `dynamic`, it is safe to > > infer a stronger type in a derived class. This won't break any users of the > > supertype, because they already have to expect it to be any possible type. > > > > (and we get `newType` from `newType = _computeReturnType(overriddenGetters);` > > above. In other words, it represents the return type of the overridden > getters) > > > > I'm happy to reword, but I don't want to just remove the comment... > > The comment has a wording problem in the first line ("if we're the field is > final"), but the gist of it makes sense to me. I'm also not sure why we only > handle the dynamic case. ah good catch. How about this: Also infer from the initialization expression if the overridden getter's return type is dynamic. This is safe to do because we will infer a stronger return type (than dynamic).
PTAL, I tried to fix the comment.
LGTM https://codereview.chromium.org/1367183004/diff/1/pkg/analyzer/lib/src/task/s... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1367183004/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:304: if (newType == null || newType.isDynamic) { > What doesn't make sense about the (original) comment? Aside from the grammatical error, it seems to me that - there is no "also", we're only doing one thing here, - the condition given for being able to infer the return type is as incomplete as the comment above, - the role of covariance is misrepresented; we could potentially infer a covariant type even if the overridden getter's type was not dynamic, but we're choosing not to. > How about this: > > Also infer from the initialization expression if the overridden getter's return > type is dynamic. > This is safe to do because we will infer a stronger return type (than dynamic). How about a single (hopefully more consistent) comment: If there is no overridden getter or if the overridden getter's type is dynamic, then we can infer the type from the initialization expression without breaking subtype rules. We could potentially infer a consistent return type even if the overridden getter's type was not dynamic, but choose not to for simplicity. The field is required to be final to prevent choosing a type that is inconsistent with assignments we cannot analyze.
https://codereview.chromium.org/1367183004/diff/1/pkg/analyzer/lib/src/task/s... File pkg/analyzer/lib/src/task/strong_mode.dart (right): https://codereview.chromium.org/1367183004/diff/1/pkg/analyzer/lib/src/task/s... pkg/analyzer/lib/src/task/strong_mode.dart:304: if (newType == null || newType.isDynamic) { On 2015/09/26 00:36:29, Brian Wilkerson wrote: > > What doesn't make sense about the (original) comment? > > Aside from the grammatical error, it seems to me that > - there is no "also", we're only doing one thing here, "if <condition>, do this" "also, if <other condition> ..." not sure why that's a problem. I didn't want to mess with the original comment, so I added "also" to explain the (IMO, unrelated) additional rule. > - the condition given for being able to infer the return type is as incomplete > as the comment above, > - the role of covariance is misrepresented; we could potentially infer a > covariant type even if the overridden getter's type was not dynamic, but we're > choosing not to. fwiw, not sure I agree with those points, I don't understand what's incomplete or misrepresented. But anyway, I can agree to disagree. Happy to use your wording. > > > How about this: > > > > Also infer from the initialization expression if the overridden getter's > return > > type is dynamic. > > This is safe to do because we will infer a stronger return type (than > dynamic). > > How about a single (hopefully more consistent) comment: > > If there is no overridden getter or if the overridden getter's type is dynamic, > then we can infer the type from the initialization expression without breaking > subtype rules. We could potentially infer a consistent return type even if the > overridden getter's type was not dynamic, but choose not to for simplicity. The > field is required to be final to prevent choosing a type that is inconsistent > with assignments we cannot analyze. Sure, I can use that. Thanks.
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 7bd3db6d741088705063373cec6b8d099225cd1c (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
