[BUGFIX lts] Fix tag cycles in query parameters

Currently, we wrap query parameters with @dependentKeyCompat manually
to ensure they work with the legacy router setup, which watches QPs
using observers. When a QP value is labeled as @Tracked, we don't need
to replace it with @dependentKeyCompat, but since it's just a plain
native getter (from the decorator) we do anyways. This results in a tag
cycle being created, which can result in a negative performance impact,
as every render pass will be invalidated by the cycle and require a
subsequent full revalidation.

This bug would have been caught by our ALLOW_CYCLES logic, but that
logic was faulty - it was allowing too many cycles, anything that was
accessed via get. It should only have allowed cycles from computeds
themselves.

This PR fixes the bug with ALLOW_CYCLES, and the subsequent bug with
@Tracked by only wrapping with @dependentKeyCompat if the value is
not @Tracked. It also adds an assertion to prevent users from using
@dependentKeyCompat with @computed or @Tracked.

Verified that the test failed after fixing ALLOW_CYCLES, and passed
again after the fix.

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

@@ -12,7 +12,7 @@ import {
   validateTag,
   valueForTag,
 } from '@glimmer/validator';
-import { finishLazyChains, getChainTagsForKey } from './chain-tags';
+import { CHAIN_PASS_THROUGH, finishLazyChains, getChainTagsForKey } from './chain-tags';
 import {
   ComputedDescriptor,
   Decorator,
@@ -70,6 +70,7 @@ export class AliasedProperty extends ComputedDescriptor {
   setup(obj: object, keyName: string, propertyDesc: PropertyDescriptor, meta: Meta): void {
     assert(`Setting alias '${keyName}' on self`, this.altKey !== keyName);
     super.setup(obj, keyName, propertyDesc, meta);
+    CHAIN_PASS_THROUGH.add(this);
   }
 
   get(obj: object, keyName: string): any {

packages/@ember/-internals/metal/lib/chain-tags.ts

@@ -1,9 +1,8 @@
 import { Meta, meta as metaFor, peekMeta } from '@ember/-internals/meta';
 import { isObject } from '@ember/-internals/utils';
 import { assert, deprecate } from '@ember/debug';
-import { DEBUG } from '@glimmer/env';
+import { _WeakSet } from '@glimmer/util';
 import {
-  ALLOW_CYCLES,
   combine,
   createUpdatableTag,
   Tag,
@@ -15,6 +14,8 @@ import {
 import { objectAt } from './array';
 import { tagForProperty } from './tags';
 
+export const CHAIN_PASS_THROUGH = new _WeakSet();
+
 export function finishLazyChains(meta: Meta, key: string, value: any) {
   let lazyTags = meta.readableLazyChainsFor(key);
 
@@ -166,7 +167,7 @@ function getChainTags(
       // If the key was an alias, we should always get the next value in order to
       // bootstrap the alias. This is because aliases, unlike other CPs, should
       // always be in sync with the aliased value.
-      if (descriptor !== undefined && typeof descriptor.altKey === 'string') {
+      if (CHAIN_PASS_THROUGH.has(descriptor)) {
         // tslint:disable-next-line: no-unused-expression
         current[segment];
       }
@@ -182,7 +183,7 @@ function getChainTags(
       } else {
         current = current[segment];
       }
-    } else if (typeof descriptor.altKey === 'string') {
+    } else if (CHAIN_PASS_THROUGH.has(descriptor)) {
       current = current[segment];
     } else {
       // If the descriptor is defined, then its a normal CP (not an alias, which
@@ -218,9 +219,5 @@ function getChainTags(
     currentMeta = peekMeta(current);
   }
 
-  if (DEBUG) {
-    chainTags.forEach(t => ALLOW_CYCLES!.set(t, true));
-  }
-
   return chainTags;
 }

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

@@ -2,8 +2,10 @@ import { Meta, meta as metaFor } from '@ember/-internals/meta';
 import { inspect, toString } from '@ember/-internals/utils';
 import { assert, deprecate, warn } from '@ember/debug';
 import EmberError from '@ember/error';
+import { DEBUG } from '@glimmer/env';
 import { isDestroyed } from '@glimmer/runtime';
 import {
+  ALLOW_CYCLES,
   consumeTag,
   tagFor,
   tagMetaFor,
@@ -321,6 +323,10 @@ export class ComputedProperty extends ComputedDescriptor {
       )
     );
 
+    if (DEBUG) {
+      ALLOW_CYCLES!.set(tagFor(obj, keyName), true);
+    }
+
     if (this._hasConfig === false) {
       assert(
         `Attempted to use @computed on ${keyName}, but it did not have a getter or a setter. You must either pass a get a function or getter/setter to @computed directly (e.g. \`@computed({ get() { ... } })\`) or apply @computed directly to a getter/setter`,

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

@@ -1,8 +1,11 @@
+import { meta as metaFor } from '@ember/-internals/meta';
 import { isEmberArray } from '@ember/-internals/utils';
 import { assert } from '@ember/debug';
 import { DEBUG } from '@glimmer/env';
 import { consumeTag, dirtyTagFor, tagFor, trackedData } from '@glimmer/validator';
+import { CHAIN_PASS_THROUGH } from './chain-tags';
 import {
+  CP_SETTER_FUNCS,
   Decorator,
   DecoratorPropertyDescriptor,
   isElementDescriptor,
@@ -142,7 +145,7 @@ if (DEBUG) {
   setClassicDecorator(tracked);
 }
 
-function descriptorForField([_target, key, desc]: [
+function descriptorForField([target, key, desc]: [
   object,
   string,
   DecoratorPropertyDescriptor
@@ -154,25 +157,36 @@ function descriptorForField([_target, key, desc]: [
 
   let { getter, setter } = trackedData<any, any>(key, desc ? desc.initializer : undefined);
 
-  return {
+  function get(this: object): unknown {
+    let value = getter(this);
+
+    // Add the tag of the returned value if it is an array, since arrays
+    // should always cause updates if they are consumed and then changed
+    if (Array.isArray(value) || isEmberArray(value)) {
+      consumeTag(tagFor(value, '[]'));
+    }
+
+    return value;
+  }
+
+  function set(this: object, newValue: unknown): void {
+    setter(this, newValue);
+    dirtyTagFor(this, SELF_TAG);
+  }
+
+  let newDesc = {
     enumerable: true,
     configurable: true,
+    isTracked: true,
 
-    get(): any {
-      let value = getter(this);
+    get,
+    set,
+  };
 
-      // Add the tag of the returned value if it is an array, since arrays
-      // should always cause updates if they are consumed and then changed
-      if (Array.isArray(value) || isEmberArray(value)) {
-        consumeTag(tagFor(value, '[]'));
-      }
+  metaFor(target).writeDescriptors(key, newDesc);
 
-      return value;
-    },
+  CP_SETTER_FUNCS.add(set);
+  CHAIN_PASS_THROUGH.add(newDesc);
 
-    set(newValue: any): void {
-      setter(this, newValue);
-      dirtyTagFor(this, SELF_TAG);
-    },
-  };
+  return newDesc;
 }

packages/@ember/object/compat.ts

@@ -2,15 +2,21 @@ import { Meta } from '@ember/-internals/meta';
 import {
   Decorator,
   DecoratorPropertyDescriptor,
+  descriptorForProperty,
   isElementDescriptor,
   setClassicDecorator,
 } from '@ember/-internals/metal';
 import { assert } from '@ember/debug';
 import { consumeTag, tagFor, track, UpdatableTag, updateTag } from '@glimmer/validator';
 
-let wrapGetterSetter = function(_target: object, key: string, desc: PropertyDescriptor) {
+let wrapGetterSetter = function(target: object, key: string, desc: PropertyDescriptor) {
   let { get: originalGet } = desc;
 
+  assert(
+    'You attempted to use @computed on a property that already has been decorated with either @computed or @tracked. @dependentKeyCompat is only necessary for native getters that are not decorated with @computed.',
+    descriptorForProperty(target, key) === undefined
+  );
+
   if (originalGet !== undefined) {
     desc.get = function() {
       let propertyTag = tagFor(this, key) as UpdatableTag;

packages/ember/tests/routing/query_params_test.js

@@ -1611,6 +1611,46 @@ moduleFor(
       this.assertCurrentPath('/?foo=987');
     }
 
+    async ['@test Single query params defined with tracked properties can be linked to (and log is present)'](
+      assert
+    ) {
+      assert.expect(3);
+
+      this.addTemplate(
+        'application',
+        `
+          <LinkTo @route="application" id="the-link">
+            Home
+          </LinkTo>
+          <LinkTo @route="application" @query={{hash foo=(array 123)}} id="the-link-with-params">
+            'Home (with params)'
+          </LinkTo>
+
+          <!-- this log caused a failure previously, so we leave it to make sure this case is tested -->
+          {{log this.foo}}
+        `
+      );
+
+      this.add(
+        `controller:application`,
+        class extends Controller {
+          queryParams = ['foo', 'bar'];
+          @tracked foo = [];
+          @tracked bar = [];
+        }
+      );
+
+      await this.visitAndAssert('/');
+
+      document.getElementById('the-link').click();
+      await runLoopSettled();
+      this.assertCurrentPath('/');
+
+      document.getElementById('the-link-with-params').click();
+      await runLoopSettled();
+      this.assertCurrentPath('/?foo=%5B123%5D');
+    }
+
     async ['@test Single query params defined with native getters and tracked properties can be on the controller and reflected in the url'](
       assert
     ) {