Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track dart:* origin of names in order to resolve conflicts #17634

Closed
DartBot opened this issue Mar 19, 2014 · 11 comments
Closed

Track dart:* origin of names in order to resolve conflicts #17634

DartBot opened this issue Mar 19, 2014 · 11 comments

Comments

@DartBot
Copy link

@DartBot DartBot commented Mar 19, 2014

This issue was originally filed by Misko.H...@gmail.com


See exception here: https://travis-ci.org/angular/angular.dart/jobs/21104418#L719

Caught type 'CssAnimation' is not a subtype of type 'Animation' of 'animation'.

The code which caused the issue is:
https://github.com/angular/angular.dart/blob/master/test/animate/css_animate_spec.dart#L114

  play(Animation animation) {
    animations.add(animation);
  }

The error says that play method is being called with CssAnimation and we expect Animation Instead.

CssAnimation is declared at: https://github.com/angular/angular.dart/blob/master/lib/animate/css_animation.dart#L9

class CssAnimation extends LoopedAnimation {
  ...
}

LoopedAnimation is declared at: https://github.com/angular/angular.dart/blob/master/lib/animate/animations.dart#L9

abstract class LoopedAnimation implements Animation {
  ...
}

Therefor CssAnimation does implement Animation and should therefore pass the type check.

This is a regression since the test fails on version 1.3.0-dev.4.1 but passes on 1.2.0

@kasperl
Copy link

@kasperl kasperl commented Mar 19, 2014

Florian has been working on this today (and can repro locally).


Set owner to @fsc8000.
Added this to the 1.3 milestone.
Removed Priority-Unassigned label.
Added Priority-High, Area-VM, Triaged labels.

@fsc8000
Copy link
Contributor

@fsc8000 fsc8000 commented Mar 20, 2014

There are two conflicting "Animation" classes in the system coming from different places: One from angular.animate, the other from dart.dom.html.

Two issues here: The VM does not report the name conflict, the other issue is where the other Animation class from dart:html comes from.

@fsc8000
Copy link
Contributor

@fsc8000 fsc8000 commented Mar 20, 2014

The following "fix" made the problem disappear: I don't know yet where the dart.dom.html.Animation comes from.

diff --git a/test/_specs.dart b/test/_specs.dart
index 75d7906..b55a137 100644

--- a/test/_specs.dart
+++ b/test/_specs.dart
@@ -8,7 +8,7 @­@ import 'package:collection/wrappers.dart' show DelegatingList;
 
 import 'jasmine_syntax.dart' as jasmine_syntax;
 
-export 'dart:html';
+export 'dart:html' hide Animation;
 export 'package:unittest/unittest.dart';
 export 'package:unittest/mock.dart';
 export 'package:di/dynamic_injector.dart';


Removed Area-VM label.
Added Area-Dartium label.
Marked this as being blocked by #17652.

@mraleph
Copy link
Member

@mraleph mraleph commented Mar 20, 2014

fwiw Animation comes from ./third_party/WebKit/Source/core/animation/Animation.idl. This interface appeared after we rolled Dartium forward to M34, it was not there before.

@vsmenon
Copy link
Member

@vsmenon vsmenon commented Mar 20, 2014

Yes, that type was added in M34. I thought type from user libs took precedence exactly for this reason. I.e., from the spec:

"If a name N is referenced by a library L and N would be introduced into the top level scope of L by an import from a library whose URI begins with dart: and an import from a library whose URI does not begin with dart::
The import from dart: is implicitly extended by a hide N clause.
A static warning is issued."

This suggests that the explicit hide Florian added should be unnecessary.


cc @blois.
cc @gbracha.
Removed Area-Dartium label.
Added Area-VM label.

@iposva-google
Copy link
Contributor

@iposva-google iposva-google commented Mar 20, 2014

Regarding comment #­5:
Vijay what you are quoting refers to imports. The thing here is that this code is exporting conflicting names.

@blois
Copy link
Contributor

@blois blois commented Mar 20, 2014

The problem is "an import from a library whose URI begins with dart:"

In this case dart:html is being exported, so the import that contains Animation is not a dart: import.

To guard against breaking changes here the logic would have to be that types originating from dart SDK libraries are shadowed rather than types introduced via dart: imports.

@kasperl
Copy link

@kasperl kasperl commented Mar 21, 2014

Marked this as being blocked by #6134.
Unmarked this as being blocked by #17652.

@kasperl
Copy link

@kasperl kasperl commented Mar 21, 2014

I think this is an issue in the spec (filed issue #17678). The Animation pulled in from the user library should be preferred over the one (indirectly) pulled in from dart:html.


Marked this as being blocked by #17678.

@iposva-google
Copy link
Contributor

@iposva-google iposva-google commented May 14, 2014

Kasper, to be clear what you want is that a name which originates in dart:* libraries, however circuitous, is hidden automatically behind the users back in case of conflicts. Is that correct?


cc @mhausner.
Removed the owner.
Removed this from the 1.3 milestone.
Removed Priority-High label.
Added Priority-Medium label.
Changed the title to: "Track dart: origin of names in order to resolve conflicts".*

@kasperl
Copy link

@kasperl kasperl commented May 14, 2014

The intent is that we can add things to a dart:* library without causing a conflict with something from a user library. To achieve this we need to make sure it works through arbitrary re-exports, so your interpretation of what the spec requires (aka what I want) is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants