Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
513563002:
Fix Issue 18628 by refixing Issue 18468 (Closed)
|
Created:
6 years, 3 months ago by Nathan Collins Modified:
6 years, 3 months ago CC:
reviews_dartlang.org, ricow1 Visibility:
Public. |
DescriptionFix Issue 18628 by refixing Issue 18468
The fix: types used as expressions have type [Type].
In r35501 @jwren committed a fix for Issue 18468, but that fix was
buggy -- it conflated type parameters with their types -- resulting in
Issue 18628. This commit includes a correct fix for Issue 18628, which
in turn fixes Issue 18628. Interestingly, the original cause of Issue
18468 seems to be another regression, but I don't have enough GitSVN
history for [git blame] to tell me anything useful :P
The tests from Issue 18628 are also included in this commit.
https://code.google.com/p/dart/issues/detail?id=18628
https://code.google.com/p/dart/issues/detail?id=18468
Revert bad fix for https://code.google.com/p/dart/issues/detail?id=18468
Partial revert, since the new test is still good, and the a good fix
shouldn't introduce any failures in other tests.
R=jwren@google.com, brianwilkerson@google.com
BUG=https://code.google.com/p/dart/issues/detail?id=18468,https://code.google.com/p/dart/issues/detail?id=18628
Committed: https://code.google.com/p/dart/source/detail?r=39630
Patch Set 1 #
Total comments: 12
Messages
Total messages: 14 (0 generated)
LGTM https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... File editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticTypeWarningCodeTest.java (right): https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticTypeWarningCodeTest.java:255: public void test_invalidAssignment_regressionInR35501() throws Exception { Change R35501 to issue number. https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticTypeWarningCodeTest.java:631: public void test_typeArgumentNotMatchingBounds_extends_regressionInR35501() throws Exception { Same here.
https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... File editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/StaticTypeAnalyzer.java (right): https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/StaticTypeAnalyzer.java:1273: staticType = typeProvider.getTypeType(); This seems like a very drastic change, and I'm surprised that the tests don't break. I know that we had a reason for making this change, but I can't think at the moment what that reason was. I'm a bit concerned that this will cause other problems, but I'll eave it to you to decide how to proceed.
collinsn@google.com changed reviewers: + gbracha@google.com, karlklose@google.com
Hi Karl and Gilad, could you take a look? The question is whether I'm correct that a type in an expression context has type [Type]. I assumed this intuitively, but I can't completely justify it based on the spec, which only gives special cases ([int], [String], [double], [bool], and [x.runtimeType] for all [x]). This implementation passes all tests, fixes the present issue (https://code.google.com/p/dart/issues/detail?id=18628), and refixes a related issue (https://code.google.com/p/dart/issues/detail?id=18468). Thanks! https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... File editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/StaticTypeAnalyzer.java (right): https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/StaticTypeAnalyzer.java:1273: staticType = typeProvider.getTypeType(); On 2014/08/27 07:42:41, Brian Wilkerson wrote: > This seems like a very drastic change, and I'm surprised that the tests don't > break. I know that we had a reason for making this change, but I can't think at > the moment what that reason was. I'm a bit concerned that this will cause other > problems, but I'll eave it to you to decide how to proceed. Can you explain why this *is not* the obvious solution? What's "drastic" about it? I'll explain why I think it *is* the obvious solution: a type parameter stands for a type, and a type in an expression context should intuitively have type [Type] (the type of types). Note: the spec does not -- that I can find anyway -- directly say that all types evaluate to instances of [Type], but it does say that - [x.runtimeType] is an instance of [Type] for all [x] - [int], [String], [bool], and [double] evaluate to an instance of [Type] - [x.runtimeType] on an [int] literal ([String], [bool], or [double], respectively) returns the [Type] instance which is the value of [int] ([String], [bool], or [double], respectively) the [Type] instance which is the value of [int] and these are special cases of the general intuition above. Let's ask @karlklose and @gbracha what they think.
Message was sent while issue was closed.
Committed patchset #1 manually as 39630 (presubmit successful).
Message was sent while issue was closed.
On 2014/08/27 06:59:40, jwren wrote: > LGTM > > https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... > File > editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticTypeWarningCodeTest.java > (right): > > https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... > editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticTypeWarningCodeTest.java:255: > public void test_invalidAssignment_regressionInR35501() throws Exception { > Change R35501 to issue number. > > https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... > editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticTypeWarningCodeTest.java:631: > public void test_typeArgumentNotMatchingBounds_extends_regressionInR35501() > throws Exception { > Same here. @jwren: I forgot to address these renames before I dcommitted; I did them in a separate CL: https://codereview.chromium.org/511053002/.
Message was sent while issue was closed.
https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... File editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/StaticTypeAnalyzer.java (right): https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/StaticTypeAnalyzer.java:1273: staticType = typeProvider.getTypeType(); On 2014/08/27 17:42:33, Nathan Collins wrote: > On 2014/08/27 07:42:41, Brian Wilkerson wrote: > > This seems like a very drastic change, and I'm surprised that the tests don't > > break. I know that we had a reason for making this change, but I can't think > at > > the moment what that reason was. I'm a bit concerned that this will cause > other > > problems, but I'll eave it to you to decide how to proceed. > > Can you explain why this *is not* the obvious solution? What's "drastic" about > it? > > I'll explain why I think it *is* the obvious solution: a type parameter stands > for a type, and a type in an expression context should intuitively have type > [Type] (the type of types). > > Note: the spec does not -- that I can find anyway -- directly say that all types > evaluate to instances of [Type], but it does say that > > - [x.runtimeType] is an instance of [Type] for all [x] > - [int], [String], [bool], and [double] evaluate to an instance of [Type] > - [x.runtimeType] on an [int] literal ([String], [bool], or [double], > respectively) returns the [Type] instance which is the value of [int] ([String], > [bool], or [double], respectively) > the [Type] instance which is the value of [int] > > and these are special cases of the general intuition above. > > Let's ask @karlklose and @gbracha what they think. As I pointed out in the review of the original change, this is the right thing to do and the previous change was incorrect. [Element.getType] looks like a overly generic method that easily leads to confusion. We had the same problem in dart2js (in particular with generic types) and decided to deprecate it and require to call a special method (rawType, thisType, functionType, bound).
Message was sent while issue was closed.
Message was sent while issue was closed.
https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... File editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticTypeWarningCodeTest.java (right): https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticTypeWarningCodeTest.java:255: public void test_invalidAssignment_regressionInR35501() throws Exception { Shouldn't we add these as language tests? Every tool that attempts to type check should be checked against these cases.
Message was sent while issue was closed.
Reply to @karlklose's comments. https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... File editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/StaticTypeAnalyzer.java (right): https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/StaticTypeAnalyzer.java:1273: staticType = typeProvider.getTypeType(); On 2014/08/28 06:54:38, karlklose wrote: > On 2014/08/27 17:42:33, Nathan Collins wrote: > > On 2014/08/27 07:42:41, Brian Wilkerson wrote: > > > This seems like a very drastic change, and I'm surprised that the tests > don't > > > break. I know that we had a reason for making this change, but I can't think > > at > > > the moment what that reason was. I'm a bit concerned that this will cause > > other > > > problems, but I'll eave it to you to decide how to proceed. > > > > Can you explain why this *is not* the obvious solution? What's "drastic" about > > it? > > > > I'll explain why I think it *is* the obvious solution: a type parameter stands > > for a type, and a type in an expression context should intuitively have type > > [Type] (the type of types). > > > > Note: the spec does not -- that I can find anyway -- directly say that all > types > > evaluate to instances of [Type], but it does say that > > > > - [x.runtimeType] is an instance of [Type] for all [x] > > - [int], [String], [bool], and [double] evaluate to an instance of [Type] > > - [x.runtimeType] on an [int] literal ([String], [bool], or [double], > > respectively) returns the [Type] instance which is the value of [int] > ([String], > > [bool], or [double], respectively) > > the [Type] instance which is the value of [int] > > > > and these are special cases of the general intuition above. > > > > Let's ask @karlklose and @gbracha what they think. > > As I pointed out in the review of the original change, this is the right thing > to do and the previous change was incorrect. > > [Element.getType] looks like a overly generic method that easily leads to > confusion. We had the same problem in dart2js (in particular with generic types) > and decided to deprecate it and require to call a special method (rawType, > thisType, functionType, bound). I don't understand what the "special method (rawType, thisType, functionType, bound)" does here. https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... File editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticTypeWarningCodeTest.java (right): https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticTypeWarningCodeTest.java:255: public void test_invalidAssignment_regressionInR35501() throws Exception { On 2014/08/28 06:59:54, karlklose wrote: > Shouldn't we add these as language tests? Every tool that attempts to type check > should be checked against these cases. Sounds good. How do I add a "language test"?
Message was sent while issue was closed.
https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... File editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/StaticTypeAnalyzer.java (right): https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/StaticTypeAnalyzer.java:1273: staticType = typeProvider.getTypeType(); On 2014/08/28 22:23:17, Nathan Collins wrote: > On 2014/08/28 06:54:38, karlklose wrote: > > On 2014/08/27 17:42:33, Nathan Collins wrote: > > > On 2014/08/27 07:42:41, Brian Wilkerson wrote: > > > > This seems like a very drastic change, and I'm surprised that the tests > > don't > > > > break. I know that we had a reason for making this change, but I can't > think > > > at > > > > the moment what that reason was. I'm a bit concerned that this will cause > > > other > > > > problems, but I'll eave it to you to decide how to proceed. > > > > > > Can you explain why this *is not* the obvious solution? What's "drastic" > about > > > it? > > > > > > I'll explain why I think it *is* the obvious solution: a type parameter > stands > > > for a type, and a type in an expression context should intuitively have type > > > [Type] (the type of types). > > > > > > Note: the spec does not -- that I can find anyway -- directly say that all > > types > > > evaluate to instances of [Type], but it does say that > > > > > > - [x.runtimeType] is an instance of [Type] for all [x] > > > - [int], [String], [bool], and [double] evaluate to an instance of [Type] > > > - [x.runtimeType] on an [int] literal ([String], [bool], or [double], > > > respectively) returns the [Type] instance which is the value of [int] > > ([String], > > > [bool], or [double], respectively) > > > the [Type] instance which is the value of [int] > > > > > > and these are special cases of the general intuition above. > > > > > > Let's ask @karlklose and @gbracha what they think. > > > > As I pointed out in the review of the original change, this is the right thing > > to do and the previous change was incorrect. > > > > [Element.getType] looks like a overly generic method that easily leads to > > confusion. We had the same problem in dart2js (in particular with generic > types) > > and decided to deprecate it and require to call a special method (rawType, > > thisType, functionType, bound). > > I don't understand what the "special method (rawType, thisType, functionType, > bound)" does here. The code you replaced referred to the "type of the element". I wanted to point out that this is easy to misuse, so it may be better to get rid of getType and have specific getters for the kind of element (for example, getBound for the type variable). https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... File editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticTypeWarningCodeTest.java (right): https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticTypeWarningCodeTest.java:255: public void test_invalidAssignment_regressionInR35501() throws Exception { On 2014/08/28 22:23:17, Nathan Collins wrote: > On 2014/08/28 06:59:54, karlklose wrote: > > Shouldn't we add these as language tests? Every tool that attempts to type > check > > should be checked against these cases. > > Sounds good. How do I add a "language test"? Add a test in tests/language. To test warnings you should create a multi-test, where you add a triple-slash ('///') comment to the line where the where the warning should appear: class C<T> { T t = int; /// 01: static type-warning } The test framework will create a version with and one without the line. The one without it should run without warnings and errors and the one with the line included should produce the result that is in the comment. Make sure to run the VM and dart2js on the test as well. They both don't type check the expression, but they would throw when executing the test above in checked mode, so you have to catch the exception (see tests/language/malbounded_type_cast2_test.dart for an example).
Message was sent while issue was closed.
https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... File editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticTypeWarningCodeTest.java (right): https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticTypeWarningCodeTest.java:255: public void test_invalidAssignment_regressionInR35501() throws Exception { On 2014/08/29 08:01:09, karlklose wrote: > On 2014/08/28 22:23:17, Nathan Collins wrote: > > On 2014/08/28 06:59:54, karlklose wrote: > > > Shouldn't we add these as language tests? Every tool that attempts to type > > check > > > should be checked against these cases. > > > > Sounds good. How do I add a "language test"? > > Add a test in tests/language. To test warnings you should create a multi-test, > where you add a triple-slash ('///') comment to the line where the where the > warning should appear: > > class C<T> { > T t = int; /// 01: static type-warning > } > > The test framework will create a version with and one without the line. The one > without it should run without warnings and errors and the one with the line > included should produce the result that is in the comment. > > Make sure to run the VM and dart2js on the test as well. They both don't type > check the expression, but they would throw when executing the test above in > checked mode, so you have to catch the exception (see > tests/language/malbounded_type_cast2_test.dart for an example). Thanks! I added the tests here: https://codereview.chromium.org/521773003 . The VM and dart2js don't seem to have any trouble in checked mode with these tests, so I did not follow the tests/language/malbounded_type_cast2_test example. But maybe I ran the VM and dart2js tests incorrectly? The way I ran them is described in the tests/README I added.
Message was sent while issue was closed.
https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... File editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticTypeWarningCodeTest.java (right): https://codereview.chromium.org/513563002/diff/1/editor/tools/plugins/com.goo... editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticTypeWarningCodeTest.java:255: public void test_invalidAssignment_regressionInR35501() throws Exception { Writing tests for the VM and dart2js is a bit tricky, as they only compile what (they think) is needed. This means that you have to actually use what you want to test. I'll add comments to the test in the other CL. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
