$inject with ES6 classes can inject the wrong dependencies

ChrisMondok commented 7 years ago

Note: for support questions, please use one of these channels: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#question. This repository's issues are reserved for feature requests and bug reports.

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

When angular annotates a base class, it uses the inherited annotations when resolving the dependencies of derived classes.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://plnkr.co or similar (template: http://plnkr.co/edit/tpl:yBpEi4).

http://plnkr.co/edit/55dt9Xx8z5qvHZQ9eDdQ?p=preview

What is the expected behavior?

Derived classes should not inherit their parent's $inject property, as they do not necessarily share dependencies.

What is the motivation / use case for changing the behavior?

This behavior introduces a potentially hard to debug issue, and is probably not what the user expects to happen.

Which versions of Angular, and which browser / OS are affected by this issue? Did this work in previous versions of Angular? Please also test with the latest stable and snapshot (https://code.angularjs.org/snapshot/) versions.

This is still an issue when running against the snapshot as of 2016-12-21

Other information (e.g. stacktraces, related issues, suggestions how to fix)

dcherman commented 7 years ago

How would this play with derived classes that do not define their own constructor? Wouldn't that fail as it would reject the use of the parent's dependencies?

https://plnkr.co/edit/v4S9ViiJIaq9jZwFU1jO?p=preview

ChrisMondok commented 7 years ago

Indeed, the attached PR would break this behavior. Personally, I would consider the fact that this behavior works now is more of a curiosity than a feature. Especially so considering that, if transpiled / minified / under strictDi, the derived class will not be given the base class's $inject property and you'll have an app that works in dev but not in prod.

ChrisMondok commented 7 years ago

Thanks for pointing that out, though, it's definitely worth noting. I'm not sure where that note goes.

dcherman commented 7 years ago

I'm pretty sure it does work on production, as I do exactly this where I work (transpiled through Babel).

ChrisMondok commented 7 years ago

Would you mind adding your comment to my PR ( #15538 ) then? Fixing this issue wouldn't necessarily break your workflow, but my PR probably would.

dcherman commented 7 years ago

Sure, done

gkalpak commented 7 years ago

Here are some thoughts (please correct me if I am wrong or missing something):

Assuming classes Parent and Child extends Parent...

Currently, the code will break only if all the following conditions are true:
1. The developer relies on implicit DI annotation.
2. Parent and Child both have constructors.
3. Parent and Child have different constructor arguments.
The solution (from the developer's standpoint) is to properly annotate Child (e.g. via $inject property, inline array, ngAnnotate etc).

There is no reliable way to detect the lack of constructor in Child. (Is there?)
If we can't distinguish an inherited constructor from a constructor without arguments, it is not possible to handle implicit annotation correctly on Child.

Based on the above, I would say that there is no way to cover all usecases with implicit annotation, so we need to pick one that won't be supported:

Auto-annotating a Child class with a constructor (and different arguments than Parent).
Auto-annotating a Child class without a constructor.

Regardless which one we choose not to support, the solution (from the developer's standpoint) will be to explicitly annotate Child classes. Except for one small (but imo important) difference:

If we keep the current implementation (not supporting (1)), the developer needs to explicitly annotate Child with the arguments of its own constructor. If we choose not to support (2), the developer needs to explicitly annotate Child with the arguments of Parent's constructor, which is more tedious and error-prone. A direct result of this difference is that in the first case (current situation), you can use a tool such as ngAnnotate to automatically annotate Child. If we choose to go the other way, then ngAnnotate won't be able to annotate Child correctly.

tl;dr Unless we can find a (reliable) way to support all usecases, I would rather keep the current implementation and document the limitation as a known issue.

mprobst commented 7 years ago

@gkalpak let's look at the failure modes.

Currently, we silently inject incorrect arguments (for non-strictDi, but also strictDi if users forget to annotate the Child class). This causes very surprising and hard to track down issues – at runtime, long after injection, suddenly something doesn't behave as expected.

If we check hasOwnProperty and require $inject to be on the child class, we avoid this issue. The failure mode is that if Child inherits the constructor from Parent and Parent has been annotated previously (manually or implicitly). The failure mode is that users receive an exception indicating they need to annotate their class.

YMMV, but I'd take explicit errors over unpredictable runtime behaviour anytime.

But maybe we can have our cake and eat it too? If we don't find a constructor in a class definition, we could walk up its Object.getPrototypeOf to handle the case correctly?

On a somewhat unrelated note, looking at the code, it seems to me as if we fail rather surprisingly for classes that don't define an explicit constructor but methods. We use Function.prototype.toString.call(Child), which for a subclass w/o a constructor gives this:

> class Parent { constructor(injected) {} }
> class Child extends Parent { myMethod(a, b) {} }
> Function.prototype.toString.call(Child)
'class Child extends Parent { myMethod(a, b) { } }'

... so AFAICT we'll try to inject a and b into Childs constructor here, which is rather wrong :-/

gkalpak commented 7 years ago

If we check hasOwnProperty and require $inject to be on the child class, we avoid this issue. [...] The failure mode is that users receive an exception indicating they need to annotate their class.

As mentioned in my previous comment, the problem with the proposed solution is that "constructor-less" derived classes will not be annotated correctly. The result will be the equivalent to:

class A { constructor(injected) { this.injected = injected } }
A.$inject = ['injected'];

class B extends A {}
B.$inject = [];   // This will be created implicitly.

In the above example, there will be no error until something tries to access this.injected (which will be undefined). So, the failure will also happen at runtime (although you can argue that debugging undefined may be easier to debug than a defined but incorrect injectable).

Unfortunately, both cases are problematic and quite similar in nature (although they fail on different usecases), but I feel the current one is easier to work around.

Of cource, if we could find a way to cover all bases, that would be great.

But maybe we can have our cake and eat it too? If we don't find a constructor in a class definition, we could walk up its Object.getPrototypeOf to handle the case correctly?

How would we look for a constructor? This issue has come up in the past and there is no easy way out. Ideally we would need a parser, but this would be an overkill, and RegExps/string manipulation isn't robust enough.

Even if we managed to fix it for native classes, this would still be an issue for transpiled code.

On a somewhat unrelated note, looking at the code, it seems to me as if we fail rather surprisingly for classes that don't define an explicit constructor but methods.

I think this is a known limitation of the current implementation, for lack of a reliable way to solve the issue. There is some discussion in #14175.

Just to be clear, I am the first to admit that the current implementation is not ideal and I am open to any sort of improvement.

mprobst commented 7 years ago

Re finding a constructor, currently we fail rather badly... maybe we could:

check if MyFn.toString() starts with class. If not, do the regular old thing
if it's a class, find the keyword constructor in the resulting string
- if constructor is not found at all ==> no constructor, check superclass or if none, zero args
- if constructor is found, execute the regexp looking for /\s+$[^)]+?$/ starting from after the constructor keyword, i.e. look for an opening paren immediately following the keyword

That's not super robust – if the keyword constructor occurs anywhere in the class body before the actual constructor, we're likely to fail. But it's a strict improvement over the current state of affairs, isn't it? There is an error case, but this will handle:

allows detection of classes not defining any constructor (mostly, that is) fixing this issue
constructor defined after another function or another code snippet that has parentheses
avoids incorrectly using the arguments to the first method for classes w/o constructors

WDYT?

mgol commented 7 years ago

@mprobst I'd rather our implicit annotations logic expected constructor to be the first method and console.warn or even throw an error otherwise (unless the annotation is explicit). Less surprises; I'm sure we'd break some code if we just looked for constructor somewhere so I'd rather avoid that.

gkalpak commented 7 years ago

Both suggestions SGTM. (And I don't think we would even break any currently working usecases.)

mprobst commented 7 years ago

@mogl so you'd search for the first parenthesis in toString(), and then check that there's a constructor keyword before it? How would you distinguish from a class that has no constructor in it?

mgol commented 7 years ago

@mprobst Good point. Since we want constructor-less classes to still work when inheriting from another class, we have 2 choices what to do if the first class method is not constructor:

Just do nothing, don't annotate anything.
Don't annotate but also console.warn advising against using classes without constructors in the implicit annotation mode.

I still think we shouldn't look for constructor further in the class stringification as we may break constructor-less classes having a function expression/declaration named constructor. It's easier to tell someone constructor must be the first method than to unbreak their code that would trip our constructor-detecting heuristic.

Doing that would fix constructor-less classes and would only break the code that works by accident; namely, having the first class method matching the first few argument names with the ones used by constructor, i.e.:

class A {
  myMethod(dep1, dep2, dep3) {}
  constructor(dep1, dep2) {/* code */}
}

Since this is quite an edge case and works only by accident, we could get away with doing that in a patch release.

WDYT?

mgol commented 7 years ago

We've diverged a little from the original issue. Going back there, I'd like whatever constructor-detecting heuristic to be used only outside of the strictDi mode as one of the strictDi purposes is to get rid of magic in favor of predictable behavior (and we recommend to enforce strictDi in development mode). We should aim at eliminating all function/class stringification by the framework (we have added a new stringification recently - the class-detecting one - but it should be gone when we drop support for turning the preAssingBindingsEnabled flag back to true) as it's too prone to error.

Because of that, I agree with @gkalpak's comment that there is no reliable way to detect constructor-less classes and that we should keep current implementation.

mhevery commented 7 years ago

The core issue is that in ES6, classes inherit not only instance properties, but also static properties. This means that static $inject on base class will be seen by the subclasses as well. This behavior can only be observed on ES6 VMs as only a VM can properly set up the prototypes so that static fields get inherited retroactively. In all transpilers the class static fields get copied during class declaration, and so any field which gets added later will not retroactively propagate to the subclasses.

What happens here is that AngularJS caches the $inject on class constructor functions. In true VMs the cache becomes visible on subclasses which causes the error described in this document, but when transpiled it works as expected because writing of the $inject on the class constructor after a child class has been declared will not cause $inject propagation to child classes.

The fix is to simply to add hasOwnProperty check in AnguarJS when the $inject cache is read. here like PR #15542

Here is the test case, and it is what all ES6 VMs will do. (The reason why transpilers don't do this is that it will turn the objects to a slow execution path, and hence its better to be fast then correct in some rarely used corner case.)

function BaseClass() {}
function ChildClass() {}
Object.setPrototypeOf(ChildClass, BaseClass)
BaseClass.$inject = [];
expect(ChildClass.$inject).toBe(BaseClass.$inject)

gkalpak commented 7 years ago

@mhevery, the problem with that is "constructor-less", derived classes (see https://github.com/angular/angular.js/issues/15536#issuecomment-268600697).

class A { constructor(a, b) {} }
A.$inject = ['a', 'b'];

class B extends A {}

With hasOwnProperty, implicit annotation won't work correctly on B.

mhevery commented 7 years ago

@gkalpak that is true, but they would not have worked even without hasOwnProperty so I think that is the best fix.

gkalpak commented 7 years ago

No, they do work (because they inherit $inject from the superclass).

mhevery commented 7 years ago

@gkalpak but that assumes that someone either decorated the superclass (in which case the should have decorated subclass as well) or that angular cached superclass, before it called subclass. Given all these restrictions I think it is kind of broken already.

gkalpak commented 7 years ago

that assumes that someone either decorated the superclass

Annotating injectables (either manually or automatically (e.g. via ngAnnotate)) is the recommended approach for real apps and that is what most people do. This would be the norm, not the exception.

(in which case the should have decorated subclass as well)

Annotating injectables without a constructor or (when there are no arguments) was never required and people usually don't do it (even in the Angular codebase we don't annotate injectables that don't have arguments).

or that angular cached superclass, before it called subclass.

Even if less common, this isn't very rare among the usecases that involve extending an injectable (which is what this issue is about). E.g. having a base controller that is used is some templates, but extending that to overwrite one method for a specific template.

Given all these restrictions I think it is kind of broken already.

As already implied above, I don't think these are real restrictions.

tl;dr

I still believe that implementing the hasOwnProperty('$inject') check will add support of a usecase, but at the expense of another equally valid/common usecase (see https://github.com/angular/angular.js/issues/15536#issuecomment-268665044).

Therefore I am slightly in favor of either implementing https://github.com/angular/angular.js/issues/15536#issuecomment-268786450 or leaving things be.

CodySchaaf commented 7 years ago

I believe I have this issue after upgrading to TypeScript 2.3. I think the issue has to do with the new __extends implementation that includes the Object.setPrototypeOf call. Now my subclassed services are not getting injected properly in dev where ngAnnotate is not active.

Original that didn't cause an issue:

var __extends = (this && this.__extends) || function (d, b) {
            for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
            function __() { this.constructor = d; }
            d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
        };

New way with the issue:

var __extends = (this && this.__extends) || (function () {
    var extendStatics = Object.setPrototypeOf ||
        ({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
        function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
    return function (d, b) {
        extendStatics(d, b);
        function __() { this.constructor = d; }
        d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
    };
})();

mgol commented 7 years ago

None of the solutions presented here got enough support to proceed with changing the status quo and the issue has seen no activity lately so I'm closing it. If someone has a concrete proposal that addresses all the mentioned issues we can reopen.

angular / angular.js

$inject with ES6 classes can inject the wrong dependencies #15536