Merge pull request #19198 from emberjs/bugfix/assert-when-setting-sha…

…dowed-property

packages/@ember/-internals/metal/lib/decorator.ts

@@ -1,5 +1,7 @@
 import { Meta, meta as metaFor, peekMeta } from '@ember/-internals/meta';
 import { assert } from '@ember/debug';
+import { DEBUG } from '@glimmer/env';
+import { _WeakSet as WeakSet } from '@glimmer/util';
 
 export type DecoratorPropertyDescriptor = (PropertyDescriptor & { initializer?: any }) | undefined;
 
@@ -69,19 +71,39 @@ export abstract class ComputedDescriptor {
   abstract set(obj: object, keyName: string, value: any | null | undefined): any | null | undefined;
 }
 
-function DESCRIPTOR_GETTER_FUNCTION(name: string, descriptor: ComputedDescriptor): () => any {
-  return function CPGETTER_FUNCTION(this: object): any {
+export let CPGETTERS: WeakSet<() => unknown>;
+export let CPSETTERS: WeakSet<(value: unknown) => void>;
+
+if (DEBUG) {
+  CPGETTERS = new WeakSet();
+  CPSETTERS = new WeakSet();
+}
+
+function DESCRIPTOR_GETTER_FUNCTION(name: string, descriptor: ComputedDescriptor): () => unknown {
+  function getter(this: object): unknown {
     return descriptor.get(this, name);
-  };
+  }
+
+  if (DEBUG) {
+    CPGETTERS.add(getter);
+  }
+
+  return getter;
 }
 
 function DESCRIPTOR_SETTER_FUNCTION(
   name: string,
   descriptor: ComputedDescriptor
-): (value: any) => void {
-  return function CPSETTER_FUNCTION(this: object, value: any): void {
+): (value: unknown) => void {
+  function setter(this: object, value: unknown): void {
     return descriptor.set(this, name, value);
-  };
+  }
+
+  if (DEBUG) {
+    CPSETTERS.add(setter);
+  }
+
+  return setter;
 }
 
 export function makeComputedDecorator(
@@ -97,10 +119,7 @@ export function makeComputedDecorator(
   ): DecoratorPropertyDescriptor {
     assert(
       `Only one computed property decorator can be applied to a class field or accessor, but '${key}' was decorated twice. You may have added the decorator to both a getter and setter, which is unnecessary.`,
-      isClassicDecorator ||
-        !propertyDesc ||
-        !propertyDesc.get ||
-        propertyDesc.get.toString().indexOf('CPGETTER_FUNCTION') === -1
+      isClassicDecorator || !propertyDesc || !propertyDesc.get || !CPGETTERS.has(propertyDesc.get)
     );
 
     let meta = arguments.length === 3 ? metaFor(target) : maybeMeta;

packages/@ember/-internals/metal/lib/property_set.ts

@@ -1,8 +1,13 @@
-import { HAS_NATIVE_PROXY, setWithMandatorySetter, toString } from '@ember/-internals/utils';
+import {
+  HAS_NATIVE_PROXY,
+  lookupDescriptor,
+  setWithMandatorySetter,
+  toString,
+} from '@ember/-internals/utils';
 import { assert } from '@ember/debug';
 import EmberError from '@ember/error';
 import { DEBUG } from '@glimmer/env';
-import { descriptorForProperty } from './decorator';
+import { CPSETTERS, descriptorForProperty } from './decorator';
 import { isPath } from './path_cache';
 import { notifyPropertyChange } from './property_events';
 import { _getPath as getPath, getPossibleMandatoryProxyValue } from './property_get';
@@ -37,7 +42,7 @@ interface ExtendedObject {
   @return {Object} the passed value.
   @public
 */
-export function set(obj: object, keyName: string, value: any, tolerant?: boolean): any {
+export function set<T = unknown>(obj: object, keyName: string, value: T, tolerant?: boolean): T {
   assert(
     `Set must be called with three or four arguments; an object, a property key, a value and tolerant true/false`,
     arguments.length === 3 || arguments.length === 4
@@ -60,7 +65,7 @@ export function set(obj: object, keyName: string, value: any, tolerant?: boolean
       `calling set on destroyed object: ${toString(obj)}.${keyName} = ${toString(value)}`,
       tolerant
     );
-    return;
+    return value;
   }
 
   if (isPath(keyName)) {
@@ -70,6 +75,17 @@ export function set(obj: object, keyName: string, value: any, tolerant?: boolean
   let descriptor = descriptorForProperty(obj, keyName);
 
   if (descriptor !== undefined) {
+    if (DEBUG) {
+      let instanceDesc = lookupDescriptor(obj, keyName);
+
+      assert(
+        `Attempted to set \`${toString(
+          obj
+        )}.${keyName}\` using Ember.set(), but the property was a computed or tracked property that was shadowed by another property declaration. This can happen if you defined a tracked or computed property on a parent class, and then redefined it on a subclass.`,
+        instanceDesc && instanceDesc.set && CPSETTERS.has(instanceDesc.set)
+      );
+    }
+
     descriptor.set(obj, keyName, value);
     return value;
   }

packages/@ember/-internals/metal/lib/tracked.ts

@@ -5,6 +5,8 @@ import { DEBUG } from '@glimmer/env';
 import { consumeTag, dirtyTagFor, tagFor, trackedData } from '@glimmer/validator';
 import { CHAIN_PASS_THROUGH } from './chain-tags';
 import {
+  CPGETTERS,
+  CPSETTERS,
   Decorator,
   DecoratorPropertyDescriptor,
   isElementDescriptor,
@@ -182,6 +184,11 @@ function descriptorForField([target, key, desc]: [
     set,
   };
 
+  if (DEBUG) {
+    CPGETTERS.add(get);
+    CPSETTERS.add(set);
+  }
+
   metaFor(target).writeDescriptors(key, new TrackedDescriptor(get, set));
 
   return newDesc;

packages/@ember/-internals/metal/tests/tracked/set_test.js

@@ -31,5 +31,21 @@ moduleFor(
         assert.equal(get(newObj, key), obj[key], 'should set value');
       }
     }
+
+    ['@test set should throw an error when setting on shadowed properties']() {
+      class Obj {
+        @tracked value = 'emberjs';
+
+        constructor() {
+          Object.defineProperty(this, 'value', { writable: true, value: 'emberjs' });
+        }
+      }
+
+      let newObj = new Obj();
+
+      expectAssertion(() => {
+        set(newObj, 'value', 123);
+      }, /Attempted to set `\[object Object\].value` using Ember.set\(\), but the property was a computed or tracked property that was shadowed by another property declaration. This can happen if you defined a tracked or computed property on a parent class, and then redefined it on a subclass/);
+    }
   }
 );

packages/@ember/-internals/routing/lib/system/router.ts

@@ -671,7 +671,7 @@ class EmberRouter extends EmberObject {
     let owner = getOwner(this);
 
     if ('string' === typeof location && owner) {
-      let resolvedLocation = owner.lookup(`location:${location}`);
+      let resolvedLocation = owner.lookup<IEmberLocation>(`location:${location}`);
 
       if (resolvedLocation !== undefined) {
         location = set(this, 'location', resolvedLocation);