Start implementing logic for determining when formal parameters need type checks.

Start implementing logic for determining when formal parameters need type checks.

We haven't quite figured out how we want to store the need for these
type checks in the kernel representation, and I'm hoping that my
coding work can help inform that decision.  So for the moment the
annotation is simply stored in the front_end wrapper
(KernelVariableDeclaration).  This is enough to get simple tests to
pass.

R=ahe@google.com

Committed: https://github.com/dart-lang/sdk/commit/6e842b3c11457b96c798a93d65d1c96571d8f57a

[Paul Berry, 2017-08-22 23:31:44]

paulberry@google.com changed reviewers:
+ ahe@google.com, scheglov@google.com

[Paul Berry, 2017-08-22 23:31:45]

I only need a review from one of you.

[ahe, 2017-08-23 09:55:58]

lgtm

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/kernel_shadow_ast.dart (right):

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_shadow_ast.dart:2390: bool isSemiSafe
= false;
As for the future direction, I suggest these are represented as annotations
similarly to how we encode native functions.

Unfortunately, we haven't yet implemented metadata on VariableDeclaration, see
issue https://github.com/dart-lang/sdk/issues/28434

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/source_loader.dart (right):

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/source_loader.dart:508: builders.forEach((Uri
uri, LibraryBuilder library) {
Can you pass in this list either from collectAllClasses or
collectAllSourceClasses in kernel_target?

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart
(right):

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart:7: import
'package:front_end/src/fasta/kernel/kernel_shadow_ast.dart';
Please don't use package import for importing from the same package.

Doing so makes it harder to visually inspect the URI for misusing references to
/src/ of another package.

In addition, it makes it really hard to compile files inside the package, see
https://github.com/dart-lang/sdk/issues/30454

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart:286: if
(!typeSchemaEnvironment.isSubtypeOf(formal.type, pessimisticType)) {
I think this needs documentation. As far as I understand, you're trying to
recognize the following situation:

class A<T> {
  foo(A argument) {}
}

Why not:

var type = formal.type;
if (!(type is TypeParameterType && type.parameter.parent == cls)) { ... }

[Paul Berry, 2017-08-23 17:09:11]

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/kernel_shadow_ast.dart (right):

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_shadow_ast.dart:2390: bool isSemiSafe
= false;
On 2017/08/23 09:55:58, ahe wrote:
> As for the future direction, I suggest these are represented as annotations
> similarly to how we encode native functions.
> 
> Unfortunately, we haven't yet implemented metadata on VariableDeclaration, see
> issue https://github.com/dart-lang/sdk/issues/28434

Let's talk with the kernel folks about this.  My gut feeling is that we
shouldn't use annotations for this, because annotations are meant for
information supplied by the user, whereas this information is supplied by the
front end and should be completely transparent to the user.

But I'm happy to go with whatever representation makes the most sense to the
kernel folks.

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/source_loader.dart (right):

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/source_loader.dart:508: builders.forEach((Uri
uri, LibraryBuilder library) {
On 2017/08/23 09:55:58, ahe wrote:
> Can you pass in this list either from collectAllClasses or
> collectAllSourceClasses in kernel_target?

Done.

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart
(right):

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart:7: import
'package:front_end/src/fasta/kernel/kernel_shadow_ast.dart';
On 2017/08/23 09:55:58, ahe wrote:
> Please don't use package import for importing from the same package.
> 
> Doing so makes it harder to visually inspect the URI for misusing references
to
> /src/ of another package.

We talked about this before and as I recall we decided that self-package imports
were preferable.  See https://codereview.chromium.org/2726793003/#msg3

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart:286: if
(!typeSchemaEnvironment.isSubtypeOf(formal.type, pessimisticType)) {
On 2017/08/23 09:55:58, ahe wrote:
> I think this needs documentation. As far as I understand, you're trying to
> recognize the following situation:
> 
> class A<T> {
>   foo(A argument) {}
> }

I assume you mean:

class A<T> {
  foo(T argument) {}
}

> 
> Why not:
> 
> var type = formal.type;
> if (!(type is TypeParameterType && type.parameter.parent == cls)) { ... }

That would be sufficient to identify the case you mention above, but it would
not identify other cases where the type parameter appears in the type of the
formal parameter in covariant fashion, e.g.:

class A<T> {
  foo(List<T> argument) {}
}

Or:

class A<T> {
  foo(T bar()) {}
}

Note however that not all uses of T in the type of foo's parameter are bad. 
This one is ok:

class A<T> {
  foo(void bar(T argument)) {}
}

I've added comments explaining the reasoning process.  In a follow up CL I'll
add additional tests to cover these corner cases.

[Paul Berry, 2017-08-23 17:17:57]

Description was changed from

==========
Start implementing logic for determining when formal parameters need type
checks.

We haven't quite figured out how we want to store the need for these
type checks in the kernel representation, and I'm hoping that my
coding work can help inform that decision.  So for the moment the
annotation is simply stored in the front_end wrapper
(KernelVariableDeclaration).  This is enough to get simple tests to
pass.
==========

to

==========
Start implementing logic for determining when formal parameters need type
checks.

We haven't quite figured out how we want to store the need for these
type checks in the kernel representation, and I'm hoping that my
coding work can help inform that decision.  So for the moment the
annotation is simply stored in the front_end wrapper
(KernelVariableDeclaration).  This is enough to get simple tests to
pass.

R=ahe@google.com

Committed:
https://github.com/dart-lang/sdk/commit/6e842b3c11457b96c798a93d65d1c96571d8f57a
==========

[Paul Berry, 2017-08-23 17:17:59]

Committed patchset #1 (id:1) manually as
6e842b3c11457b96c798a93d65d1c96571d8f57a (presubmit successful).

[ahe, 2017-08-24 09:23:48]

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart
(right):

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart:7: import
'package:front_end/src/fasta/kernel/kernel_shadow_ast.dart';
On 2017/08/23 17:09:10, Paul Berry wrote:
> On 2017/08/23 09:55:58, ahe wrote:
> > Please don't use package import for importing from the same package.
> > 
> > Doing so makes it harder to visually inspect the URI for misusing references
> to
> > /src/ of another package.
> 
> We talked about this before and as I recall we decided that self-package
imports
> were preferable.  See https://codereview.chromium.org/2726793003/#msg3

No, we didn't decide that. You argued for using package imports, and I moved all
main methods out of lib/.

In fact, I've been very consistent about not using package imports to front_end
in the fasta directory. And that's a popular style:

ls pkg/*/lib/*.dart third_party/pkg/*/lib/*.dart | xargs grep -E "^import " |
grep package:

In addition, the style guide doesn't support you your statement that "... we
decided to do instead was to tell people to always use a "package:" URI when
referring to a file in "lib/" ..."

This is from https://www.dartlang.org/tools/pub/package-layout:

Tip for web apps: For the best performance when developing with dartdevc, put
implementation files under /lib/src, instead of elsewhere under /lib. Also,
avoid imports of package:package_name/src/....

And:

When you use libraries from within your own package, even code in src, you can
(and should) still use package: to import them. 

I don't think the consensus exists. I think the analysis_server is the cause of
the confusion, and frankly it should be fixed to avoid exacerbating this
problem: it is really hard to read code reviews when you have to stop and
analyze if an import of /src/ is valid.

[ahe, 2017-08-24 09:35:26]

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart
(right):

https://codereview.chromium.org/3000353002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/type_inference/type_inference_engine.dart:286: if
(!typeSchemaEnvironment.isSubtypeOf(formal.type, pessimisticType)) {
On 2017/08/23 17:09:10, Paul Berry wrote:
> On 2017/08/23 09:55:58, ahe wrote:
> > I think this needs documentation. As far as I understand, you're trying to
> > recognize the following situation:
> > 
> > class A<T> {
> >   foo(A argument) {}
> > }
> 
> I assume you mean:
> 
> class A<T> {
>   foo(T argument) {}
> }

Yes.

> > 
> > Why not:
> > 
> > var type = formal.type;
> > if (!(type is TypeParameterType && type.parameter.parent == cls)) { ... }
> 
> That would be sufficient to identify the case you mention above, but it would
> not identify other cases where the type parameter appears in the type of the
> formal parameter in covariant fashion, e.g.:
> 
> class A<T> {
>   foo(List<T> argument) {}
> }
> 
> Or:
> 
> class A<T> {
>   foo(T bar()) {}
> }
> 
> Note however that not all uses of T in the type of foo's parameter are bad. 
> This one is ok:
> 
> class A<T> {
>   foo(void bar(T argument)) {}
> }
> 
> I've added comments explaining the reasoning process.  In a follow up CL I'll
> add additional tests to cover these corner cases.

Thank you, I looked at your comments and it now makes sense to me. Next time, it
would be nice if you ensure that what you commit is uploaded as new patch set
(not that it needs another lgtm, but it's easier to comment if needed).