Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

Infer parameter types #105

Closed
jacob314 opened this issue Mar 26, 2015 · 8 comments
Closed

Infer parameter types #105

jacob314 opened this issue Mar 26, 2015 · 8 comments

Comments

@jacob314
Copy link
Contributor

Currently we infer return types but not parameter types.
We will have an easiest to explain story for users if we infer both.

Example errors currently reported if parameter types are omitted in a subclass. Technically these errors shouldn't occur but in practice I think they are good as they match the user intent that a subclass rarely intentionally uses a less specific type than the base class.

severe: line 9, column 3 of package:angular/change_detection/prototype_map.dart: [InvalidMethodOverride] Invalid override. The type of PrototypeMap.[]= ((dynamic, dynamic) → void) is not a subtype of Map<K, V>.[]= ((K, V) → void).
  void operator []=(name, value) {
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
severe: line 22, column 3 of package:angular/change_detection/prototype_map.dart: [InvalidMethodOverride] Invalid override. The type of PrototypeMap.forEach ((dynamic) → void) is not a subtype of Map<K, V>.forEach (((K, V) → void) → void).
  void forEach(fn) {

Repro source code diff to fix these errors:
For example, consider the following diff required to fix:

   PrototypeMap(this.prototype);

-  void operator []=(name, value) {
+  void operator []=(K name, V value) {
     self[name] = value;
   }
-  V operator [](name) => self.containsKey(name) ? self[name] : prototype[name];
+  V operator [](Object name) => self.containsKey(name) ? self[name] : prototype[name];

   bool get isEmpty => self.isEmpty && prototype.isEmpty;
   bool get isNotEmpty => self.isNotEmpty || prototype.isNotEmpty;
 @@ -19,21 +19,21 @@ class PrototypeMap<K, V> implements Map<K,V> {
   Iterable<V> get values => self.values;
   int get length => self.length;

-  void forEach(fn) {
+  void forEach(void fn(K key, V value)) {
     // todo(vbe) include prototype ?
     self.forEach(fn);
   }
-  V remove(key) => self.remove(key);
+  V remove(Object key) => self.remove(key);
   clear() => self.clear;
   // todo(vbe) include prototype ?
-  bool containsKey(key) => self.containsKey(key);
+  bool containsKey(Object key) => self.containsKey(key);
   // todo(vbe) include prototype ?
-  bool containsValue(key) => self.containsValue(key);
-  void addAll(map) {
+  bool containsValue(Object value) => self.containsValue(value);
+  void addAll(Map<K, V> map) {
     self.addAll(map);
   }
   // todo(vbe) include prototype ?
-  V putIfAbsent(key, fn) => self.putIfAbsent(key, fn);
+  V putIfAbsent(K key, V fn()) => self.putIfAbsent(key, fn);

   toString() => self.toString();
 }
@jmesserly
Copy link
Contributor

@leafpetersen @vsmenon dumb question: isn't it perfectly safe to override void operator []=(K name, V value) { with void operator []=(name, value) { ? the latter accepts more things, so it can be used anywhere the base type is used without requiring extra checks.

I'm even more confused by V remove(key) vs V remove(Object key) ...

[edit: fixed to use vsmenon instead of vsm ... sorry!]

@sigmundch
Copy link
Contributor

@vsmenon
+1 to John's question. This seems to have changed recently with our changes about fuzzy-types.

Regarding inference, we initially didn't want to do inference of dynamic on the arguments of subclasses because this was a valid override and we were only doing inference if not doing so would have yielded an invalid override error. We could add inference as well, but I honestly would like this override to be allowed too.

@jacob314
Copy link
Contributor Author

@jmesserly you are correct.
It is safe but the compiler currently gets slightly confused and thinks it isn't. The issue is that it doesn't really match the user's intent.
If I wrote

void operator [](name, value)

I really meant

void operator [](K name, V value)

If i want to the method to have a different signature than the base class I could always write

void operator [](dynamic name, dynamic value)

@vsmenon
Copy link
Contributor

vsmenon commented Mar 26, 2015

I filed an issue for the typing bug (#107).

We can keep this issue on whether or when we should infer parameter types.

@vsmenon
Copy link
Contributor

vsmenon commented Mar 26, 2015

One counterexample of sorts:

class A {
void foo(x) { ... }
}

class B {
void foo(T x) { ... }
}

Our current behavior allows this. Typing foo as Object->void would not.

@jmesserly
Copy link
Contributor

Is this issue still open?

@leafpetersen
Copy link
Contributor

Yep. This is dependent on either folding the inference into the checker or doing it before the checker runs, which is on my todo list.

@vsmenon
Copy link
Contributor

vsmenon commented Aug 12, 2015

Note, this is coming up frequently in angular runs:

https://travis-ci.org/angular/angular/jobs/75265458

E.g.,

dist/dart/angular2:[ERROR] Invalid override. The type of Html5LibDomAdapter.resolveAndSetHref ((dynamic, dynamic, dynamic) → void) is not a subtype of DomAdapter.resolveAndSetHref ((dynamic, String, String) → dynamic). (/home/travis/build/angular/angular/dist/dart/angular2/lib/src/dom/html_adapter.dart, line 371, col 3)

The omitted parameter types in the override could be inferred as String here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

5 participants