Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9835007:
Typecheck constructor calls. I'm pretty sure about the call to (Closed)
|
Created:
8 years, 9 months ago by polux Modified:
8 years, 8 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionTypecheck constructor calls.
Committed: https://code.google.com/p/dart/source/detail?r=6040
Patch Set 1 #
Total comments: 12
Patch Set 2 : take peter's commments into account #
Total comments: 4
Patch Set 3 : make sure we don't return before typechecking the arguments #
Total comments: 4
Patch Set 4 : fix lookupMethodType #
Total comments: 4
Patch Set 5 : Peter's comments #
Total comments: 4
Patch Set 6 : fix logic #Patch Set 7 : Peter's comments #Patch Set 8 : Trailing space #Patch Set 9 : Add tests to TypeCheckerTest #
Total comments: 4
Patch Set 10 : Karl's comments #Patch Set 11 : trailing whitespace #
Messages
Total messages: 22 (0 generated)
I have some suggestions for how to simplify this. https://chromiumcodereview.appspot.com/9835007/diff/1/frog/leg/typechecker.dart File frog/leg/typechecker.dart (right): https://chromiumcodereview.appspot.com/9835007/diff/1/frog/leg/typechecker.da... frog/leg/typechecker.dart:376: void checkArgumentTypes(Node sendOrNew, Link<Node> argumentNodes, Change this to: checkArgumentTypes(Send send, FunctionType funType) { Link<Type> argumentTypes = analyzeArguments(node.arguments); if (funType === null) return; Link<Type> parameterTypes = funType.parameterTypes; ... https://chromiumcodereview.appspot.com/9835007/diff/1/frog/leg/typechecker.da... frog/leg/typechecker.dart:442: Link<Type> argumentTypes = analyzeArguments(node.arguments); Move this check to line 473. https://chromiumcodereview.appspot.com/9835007/diff/1/frog/leg/typechecker.da... frog/leg/typechecker.dart:446: if (receiverType === types.dynamicType) return types.dynamicType; Don't return here, but skip the rest. https://chromiumcodereview.appspot.com/9835007/diff/1/frog/leg/typechecker.da... frog/leg/typechecker.dart:457: if (memberType === types.dynamicType) return types.dynamicType; Don't return here. https://chromiumcodereview.appspot.com/9835007/diff/1/frog/leg/typechecker.da... frog/leg/typechecker.dart:474: checkArgumentTypes(node, node.arguments, funType.parameterTypes, argumentTypes); Change this to: checkArgumentTypes(node, funType); https://chromiumcodereview.appspot.com/9835007/diff/1/frog/leg/typechecker.da... frog/leg/typechecker.dart:525: FunctionType funtype = computeType(element); Calling computeType resolves the element on-demand. So this is fine. https://chromiumcodereview.appspot.com/9835007/diff/1/frog/leg/typechecker.da... frog/leg/typechecker.dart:526: checkArgumentTypes(node, node.send.arguments, funtype.parameterTypes, argumentTypes); Change this to: checkArgumentTypes(node.send, funType);
https://chromiumcodereview.appspot.com/9835007/diff/1/frog/leg/typechecker.dart File frog/leg/typechecker.dart (right): https://chromiumcodereview.appspot.com/9835007/diff/1/frog/leg/typechecker.da... frog/leg/typechecker.dart:376: void checkArgumentTypes(Node sendOrNew, Link<Node> argumentNodes, I could as well merge the loops of checkArgumentTypes and analyzeArguments into one single method then. That's what I wanted to avoid first, but if analyzeArguments is only called by checkArgumentTypes, we can avoid an intermediate list there.
https://chromiumcodereview.appspot.com/9835007/diff/1/frog/leg/typechecker.dart File frog/leg/typechecker.dart (right): https://chromiumcodereview.appspot.com/9835007/diff/1/frog/leg/typechecker.da... frog/leg/typechecker.dart:376: void checkArgumentTypes(Node sendOrNew, Link<Node> argumentNodes, On 2012/03/22 15:08:26, polux wrote: > I could as well merge the loops of checkArgumentTypes and analyzeArguments into > one single method then. That's what I wanted to avoid first, but if > analyzeArguments is only called by checkArgumentTypes, we can avoid an > intermediate list there. Done. https://chromiumcodereview.appspot.com/9835007/diff/1/frog/leg/typechecker.da... frog/leg/typechecker.dart:474: checkArgumentTypes(node, node.arguments, funType.parameterTypes, argumentTypes); On 2012/03/22 14:46:23, ahe wrote: > Change this to: > > checkArgumentTypes(node, funType); Done. https://chromiumcodereview.appspot.com/9835007/diff/1/frog/leg/typechecker.da... frog/leg/typechecker.dart:525: FunctionType funtype = computeType(element); On 2012/03/22 14:46:23, ahe wrote: > Calling computeType resolves the element on-demand. So this is fine. Done. https://chromiumcodereview.appspot.com/9835007/diff/1/frog/leg/typechecker.da... frog/leg/typechecker.dart:526: checkArgumentTypes(node, node.send.arguments, funtype.parameterTypes, argumentTypes); On 2012/03/22 14:46:23, ahe wrote: > Change this to: > > checkArgumentTypes(node.send, funType); Done.
LGTM provided you address the returns. Please also wait for Karl's comments. https://chromiumcodereview.appspot.com/9835007/diff/2002/frog/leg/typechecker... File frog/leg/typechecker.dart (right): https://chromiumcodereview.appspot.com/9835007/diff/2002/frog/leg/typechecker... frog/leg/typechecker.dart:437: if (receiverType === types.dynamicType) return types.dynamicType; Don't return. https://chromiumcodereview.appspot.com/9835007/diff/2002/frog/leg/typechecker... frog/leg/typechecker.dart:448: if (memberType === types.dynamicType) return types.dynamicType; Don't return.
https://chromiumcodereview.appspot.com/9835007/diff/2002/frog/leg/typechecker... File frog/leg/typechecker.dart (right): https://chromiumcodereview.appspot.com/9835007/diff/2002/frog/leg/typechecker... frog/leg/typechecker.dart:437: if (receiverType === types.dynamicType) return types.dynamicType; On 2012/03/22 15:48:03, ahe wrote: > Don't return. Done. https://chromiumcodereview.appspot.com/9835007/diff/2002/frog/leg/typechecker... frog/leg/typechecker.dart:448: if (memberType === types.dynamicType) return types.dynamicType; On 2012/03/22 15:48:03, ahe wrote: > Don't return. Done.
Still LGTM, but let's wait for Karl. https://chromiumcodereview.appspot.com/9835007/diff/5002/frog/leg/typechecker... File frog/leg/typechecker.dart (right): https://chromiumcodereview.appspot.com/9835007/diff/5002/frog/leg/typechecker... frog/leg/typechecker.dart:450: fail(node, 'can only handle function types'); return null;
http://codereview.chromium.org/9835007/diff/5002/frog/leg/typechecker.dart File frog/leg/typechecker.dart (right): http://codereview.chromium.org/9835007/diff/5002/frog/leg/typechecker.dart#ne... frog/leg/typechecker.dart:350: if (member !== null) return computeType(member); This is broken. It should be: if (member !== null) break; http://codereview.chromium.org/9835007/diff/5002/frog/leg/typechecker.dart#ne... frog/leg/typechecker.dart:449: if (memberType is !FunctionType) { Then you can remove this check.
Fixed lookupMethodType so that it always complains when we lookup a non method and always returns the actual type (even if not functional). https://chromiumcodereview.appspot.com/9835007/diff/5002/frog/leg/typechecker... File frog/leg/typechecker.dart (right): https://chromiumcodereview.appspot.com/9835007/diff/5002/frog/leg/typechecker... frog/leg/typechecker.dart:450: fail(node, 'can only handle function types'); Done. Also fixed lookupMethodType as discussed. On 2012/03/22 16:31:01, ahe wrote: > return null;
https://chromiumcodereview.appspot.com/9835007/diff/7001/frog/leg/typechecker... File frog/leg/typechecker.dart (right): https://chromiumcodereview.appspot.com/9835007/diff/7001/frog/leg/typechecker... frog/leg/typechecker.dart:355: [classElement.name, name]); Once you have reported a warning, please return dynamic type. You don't want the computation to continue. https://chromiumcodereview.appspot.com/9835007/diff/7001/frog/leg/typechecker... frog/leg/typechecker.dart:450: if (memberType is !FunctionType) return null; Then this case can't happen.
https://chromiumcodereview.appspot.com/9835007/diff/7001/frog/leg/typechecker... File frog/leg/typechecker.dart (right): https://chromiumcodereview.appspot.com/9835007/diff/7001/frog/leg/typechecker... frog/leg/typechecker.dart:355: [classElement.name, name]); On 2012/03/22 18:48:37, ahe wrote: > Once you have reported a warning, please return dynamic type. You don't want the > computation to continue. Done. https://chromiumcodereview.appspot.com/9835007/diff/7001/frog/leg/typechecker... frog/leg/typechecker.dart:450: if (memberType is !FunctionType) return null; On 2012/03/22 18:48:37, ahe wrote: > Then this case can't happen. Done.
LGTM if Karl approves. https://chromiumcodereview.appspot.com/9835007/diff/5/frog/leg/typechecker.dart File frog/leg/typechecker.dart (right): https://chromiumcodereview.appspot.com/9835007/diff/5/frog/leg/typechecker.da... frog/leg/typechecker.dart:352: if (member !== null) { I think you could just restore the old version, right? Or am I overlooking something? https://chromiumcodereview.appspot.com/9835007/diff/5/tests/language/src/Call... File tests/language/src/CallNonMethodFieldTest.dart (right): https://chromiumcodereview.appspot.com/9835007/diff/5/tests/language/src/Call... tests/language/src/CallNonMethodFieldTest.dart:16: x1.i(); /// 01: static type warning Whoops. This will cause a runtime error. The simplest fix I can think of is to wrap it all in if (false) { ... }
https://chromiumcodereview.appspot.com/9835007/diff/5/frog/leg/typechecker.dart File frog/leg/typechecker.dart (right): https://chromiumcodereview.appspot.com/9835007/diff/5/frog/leg/typechecker.da... frog/leg/typechecker.dart:352: if (member !== null) { Ah sorry, I totally screwed up the logic there. On 2012/03/22 21:04:59, ahe wrote: > I think you could just restore the old version, right? Or am I overlooking > something? https://chromiumcodereview.appspot.com/9835007/diff/5/tests/language/src/Call... File tests/language/src/CallNonMethodFieldTest.dart (right): https://chromiumcodereview.appspot.com/9835007/diff/5/tests/language/src/Call... tests/language/src/CallNonMethodFieldTest.dart:16: x1.i(); /// 01: static type warning On 2012/03/22 21:04:59, ahe wrote: > Whoops. This will cause a runtime error. The simplest fix I can think of is to > wrap it all in if (false) { ... } Done.
SLGTM
LGTM. The test case looks to me like it belonged in TypeAnalyzerTest. Is there currently no language test that tests for these warnings? It seems to me that it would be legitimate for a compiler to skip compiling the then-parts of the ifs (because it knows that the condition is false) and thus it would not generate any warning at all. @ahe: if the test specifies type 'static type warning', is a runtime error a test failure?
On 2012/03/23 17:05:27, karlklose wrote: > LGTM. > > The test case looks to me like it belonged in TypeAnalyzerTest. Is there > currently no language test that tests for these warnings? > It seems to me that it would be legitimate for a compiler to skip compiling the > then-parts of the ifs (because it knows that the condition is false) and thus it > would not generate any warning at all. As I understand from a discussion with Peter, there would be two modes for dart2js: one that may skip dead code in the type checking phase, and one that may not (for instance, when called by an IDE). Can't we assume these tests are using the second mode? > @ahe: if the test specifies type 'static > type warning', is a runtime error a test failure?
On 2012/03/26 09:24:33, polux wrote: > On 2012/03/23 17:05:27, karlklose wrote: > > LGTM. > > > > The test case looks to me like it belonged in TypeAnalyzerTest. Is there > > currently no language test that tests for these warnings? > > It seems to me that it would be legitimate for a compiler to skip compiling > the > > then-parts of the ifs (because it knows that the condition is false) and thus > it > > would not generate any warning at all. > > As I understand from a discussion with Peter, there would be two modes for > dart2js: one that may skip dead code in the type checking phase, and one that > may not (for instance, when called by an IDE). Can't we assume these tests are > using the second mode? I don't think the type checker should ignore dead code in a method. However, the type checker may never see a method if the method is not compiled. > > > @ahe: if the test specifies type 'static > > type warning', is a runtime error a test failure? Yes.
Added tests to TypeCheckerTest. Because the other files have been moved by https://chromiumcodereview.appspot.com/9873021/ it looks like there's a diff with patch set 8 for them too but that's not the case. On 2012/03/26 09:28:38, ahe wrote: > On 2012/03/26 09:24:33, polux wrote: > > On 2012/03/23 17:05:27, karlklose wrote: > > > LGTM. > > > > > > The test case looks to me like it belonged in TypeAnalyzerTest. Is there > > > currently no language test that tests for these warnings? > > > It seems to me that it would be legitimate for a compiler to skip compiling > > the > > > then-parts of the ifs (because it knows that the condition is false) and > thus > > it > > > would not generate any warning at all. > > > > As I understand from a discussion with Peter, there would be two modes for > > dart2js: one that may skip dead code in the type checking phase, and one that > > may not (for instance, when called by an IDE). Can't we assume these tests are > > using the second mode? > > I don't think the type checker should ignore dead code in a method. However, the > type checker may never see a method if the method is not compiled. > > > > > > @ahe: if the test specifies type 'static > > > type warning', is a runtime error a test failure? > > Yes.
Still LGTM. Thanks for the tests. https://chromiumcodereview.appspot.com/9835007/diff/10001/frog/tests/leg/src/... File frog/tests/leg/src/TypeCheckerTest.dart (right): https://chromiumcodereview.appspot.com/9835007/diff/10001/frog/tests/leg/src/... frog/tests/leg/src/TypeCheckerTest.dart:144: analyze("new C1();", I think most of these would fit on one line (also in testConstructorInvocationArgumentTypes and testMethodInvocationArgumentCount). https://chromiumcodereview.appspot.com/9835007/diff/10001/frog/tests/leg/src/... frog/tests/leg/src/TypeCheckerTest.dart:152: analyze("new C1();", new C1() -> new C2() (also in l. 154 and 156). Please also add a positive test like in line 144.
On 2012/03/30 08:55:31, karlklose wrote: > Still LGTM. Thanks for the tests. > > https://chromiumcodereview.appspot.com/9835007/diff/10001/frog/tests/leg/src/... > File frog/tests/leg/src/TypeCheckerTest.dart (right): > > https://chromiumcodereview.appspot.com/9835007/diff/10001/frog/tests/leg/src/... > frog/tests/leg/src/TypeCheckerTest.dart:144: analyze("new C1();", > I think most of these would fit on one line (also in > testConstructorInvocationArgumentTypes and testMethodInvocationArgumentCount). > > https://chromiumcodereview.appspot.com/9835007/diff/10001/frog/tests/leg/src/... > frog/tests/leg/src/TypeCheckerTest.dart:152: analyze("new C1();", > new C1() -> new C2() (also in l. 154 and 156). Please also add a positive test > like in line 144.
https://chromiumcodereview.appspot.com/9835007/diff/10001/frog/tests/leg/src/... File frog/tests/leg/src/TypeCheckerTest.dart (right): https://chromiumcodereview.appspot.com/9835007/diff/10001/frog/tests/leg/src/... frog/tests/leg/src/TypeCheckerTest.dart:144: analyze("new C1();", Done, I thought the layout of the other methods was like that on purpose. On 2012/03/30 08:55:31, karlklose wrote: > I think most of these would fit on one line (also in > testConstructorInvocationArgumentTypes and testMethodInvocationArgumentCount). https://chromiumcodereview.appspot.com/9835007/diff/10001/frog/tests/leg/src/... frog/tests/leg/src/TypeCheckerTest.dart:152: analyze("new C1();", On 2012/03/30 08:55:31, karlklose wrote: > new C1() -> new C2() (also in l. 154 and 156). Thanks for catching that. > Please also add a positive test > like in line 144. Actually, I kind of did but before my comment and with C1... what a mess. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
