Minor adjustments to vm patch files

Minor adjustments to patch files

The implementation of patching in analyzer checks that signatures are identical.
This is stricter than it needs to be, but the required fixes in dart:core are
pretty simple.

BUG=
R=hausner@google.com

Committed: https://github.com/dart-lang/sdk/commit/053e0ee720bf6963a11d9b498b3e5b97984dfff9

[Siggi Cherem (dart-lang), 2017-01-27 21:10:08]

Description was changed from

==========
Minor adjustments to patch files

The implementation of patching in analyzer checks that signatures are identical.
This is stricter than it needs to be, but the required fixes in dart:core are
pretty simple.

BUG=
==========

to

==========
Minor adjustments to patch files

The implementation of patching in analyzer checks that signatures are identical.
This is stricter than it needs to be, but the required fixes in dart:core are
pretty simple.

BUG=
==========

[Siggi Cherem (dart-lang), 2017-01-27 21:10:12]

sigmund@google.com changed reviewers:
+ hausner@google.com

[hausner, 2017-01-27 21:47:56]

lgtm

The Analyzer is broken if a function with no declared return type is not
considered to be of type dynamic. Would it not make more sense to fix the
Analyzer?

https://codereview.chromium.org/2655223005/diff/1/runtime/lib/object_patch.dart
File runtime/lib/object_patch.dart (right):

https://codereview.chromium.org/2655223005/diff/1/runtime/lib/object_patch.da...
runtime/lib/object_patch.dart:46: @patch dynamic noSuchMethod(Invocation
invocation) {
This should not be necessary; the type of a function with no explicit return
type is 'dynamic'.

[Siggi Cherem (dart-lang), 2017-01-27 22:04:03]

Agree, in fact both changes are not technically needed and could be addressed in
the analyzer patching algorithm.

Paul - it seems that these two cases could be detected and accepted. Do you
agree? I know that you wanted to keep the algorithm completely syntactic, I'm
guessing this is still within the scope of that?

Matthias, if you don't mind, I might like to submit this for now and then follow
up with fixes in analyzer.

[Siggi Cherem (dart-lang), 2017-01-27 22:21:31]

On 2017/01/27 22:04:03, Siggi Cherem (dart-lang) wrote:
> Agree, in fact both changes are not technically needed and could be addressed
in
> the analyzer patching algorithm.
> 
> Paul - it seems that these two cases could be detected and accepted. Do you
> agree? I know that you wanted to keep the algorithm completely syntactic, I'm
> guessing this is still within the scope of that?
> 
> Matthias, if you don't mind, I might like to submit this for now and then
follow
> up with fixes in analyzer.

I filed https://github.com/dart-lang/sdk/issues/28542 to track this.

Thinking more about this, I believe this might have been done on purpose because
of strong-mode semantics.  Without strong semantics dynamic and no-type means
the same thing, but in strong-mode it might not. Strong-mode performs type
inference when types are not specified and may fill in some other type. IIRC,
Paul's goal was to not dive into these semantic issues by making the patching a
bit stricter.

[Siggi Cherem (dart-lang), 2017-01-27 22:24:13]

Description was changed from

==========
Minor adjustments to patch files

The implementation of patching in analyzer checks that signatures are identical.
This is stricter than it needs to be, but the required fixes in dart:core are
pretty simple.

BUG=
==========

to

==========
Minor adjustments to patch files

The implementation of patching in analyzer checks that signatures are identical.
This is stricter than it needs to be, but the required fixes in dart:core are
pretty simple.

BUG=
R=hausner@google.com

Committed:
https://github.com/dart-lang/sdk/commit/053e0ee720bf6963a11d9b498b3e5b97984dfff9
==========

[Siggi Cherem (dart-lang), 2017-01-27 22:24:14]

Committed patchset #1 (id:1) manually as
053e0ee720bf6963a11d9b498b3e5b97984dfff9 (presubmit successful).

[hausner, 2017-01-27 22:24:34]

I gave an LGTM the first time around. Feel free to submit.

[Paul Berry, 2017-01-27 22:34:17]

On 2017/01/27 22:04:03, Siggi Cherem (dart-lang) wrote:
> Agree, in fact both changes are not technically needed and could be addressed
in
> the analyzer patching algorithm.
> 
> Paul - it seems that these two cases could be detected and accepted. Do you
> agree? I know that you wanted to keep the algorithm completely syntactic, I'm
> guessing this is still within the scope of that?
> 
> Matthias, if you don't mind, I might like to submit this for now and then
follow
> up with fixes in analyzer.

Regarding the case of mismatched argument names, we could fix it in the analyzer
patching algorithm, but it would be non-ideal: to avoid name collisions, we
would have to change the argument name in the signature of the SDK function to
match the argument name in the patch; that argument name change would then leak
out and be visible through error/warning messages and through reflection.  E.g.
imagine a core function f declared like this:

external void f(int x);

and patched like this:

@patch
void f(int arg);

If a user tried to pass the wrong type to f in a patched SDK, we would issue an
warning like "wrong type passed to argument `arg` of f", but when the user
navigated to the definition of f in their editor, they would navigate to the
external declaration (since analysis server doesn't know which platform to patch
in), so the argument would be called `x`.

Also, fixing the analyzer to accept mismatched names is non-trivial: currently
analyzer checks that the signatures match by just comparing them token by token.
 To make it accept the mismatched names, we would have to actually recurse
through the AST structures distinguishing the types from the names, and copying
the names over.  And of course arguments enclosed in "{}" would still be
required to match.

Neither of these are a huge deal, of course.  But then again neither is this
trivial fix to the patch files.  Personally I'd prefer to go ahead with Siggi's
fix rather than have more complex patching logic and slightly confusing error
messages/reflection info.


Regarding the missing return type, it is true that in this case a missing return
type means "dynamic", but it's not true for methods in general, because strong
mode type inference will sometimes infer a different return type due to
inheritance.  Since the patching logic operates prior to resolution, the
analyzer doesn't have the information to know what return type strong mode would
have inferred, so it can't tell whether the patch is safe.  We could add special
case logic to the analyzer that says "when patching a class called 'Object',
assume that a missing return type means 'dynamic'", but that sounds like a
really nasty kludge to me.  In this case my personal preference would again be
to go ahead with Siggi's trivial change rather than introduce a kludge to the
patching logic.

[hausner, 2017-01-28 07:12:35]

I agree: no kludges, please.

Thanks for the explanation, Paul.