What should be put into AssertedParameterScope for duplicate parameters?

[arai-a]

from https://bugzilla.mozilla.org/show_bug.cgi?id=1497788

If there's duplicate parameters, what should be in AssertedParameterScope?

The current spec requires AssertedPositionalParameterName for them. like,

function f(a, a) {}

will have the following data for AssertedParameterScope:

AssertedParameterScope {
  paramNames: [
    AssertedPositionalParameterName {
      index: 0,
      name: "a",
      ...
    },
    AssertedPositionalParameterName {
      index: 1,
      name: "a",
      ...
    },
  ],
  ...
}

given the purpose of Asserted*Scope is to provide the information about binding, having duplicate entry without any info about duplication won't be nice.

The situation is following:

• duplicate parameters are allowed only if there's no destructuring/default/rest parameters
• only the last one of the duplicate parameters become the actual binding

what I can think of is the following 2 solutions:

(a) Use yet another Asserted*Name with index for non-last duplicate parameters

for example, AssertedPositionalDuplicateParameterName

AssertedParameterScope {
  paramNames: [
    AssertedPositionalDuplicateParameterName {
      index: 0,
      name: "a",
      ...
    },
    AssertedPositionalParameterName {
      index: 1,
      name: "a",
      ...
    },
  ],
  ...
}

this way, CheckParameterNames and CheckPositionalParameterIndices can be done in almost same way as current ones

(b) Do not put non-last duplicate parameters

AssertedParameterScope {
  paramNames: [
    AssertedPositionalParameterName {
      index: 1,
      name: "a",
      ...
    },
  ],
  ...
}

this is smallest, but CheckParameterNames and CheckPositionalParameterIndices should be modified in order to check duplication

[Yoric]

Quick remark: As far as compression or performance is concerned, we probably don't care about optimizing this case. It's generally an error and certainly unusual.

[efaust]

I think we might be overcomplicating things? We know that it's OK to have redundant positional parameters in certain cases, and we need to parse the AssertedParameterScope all at once. Is there some reason we can't leave the duplicate entries and just update the index in our data structures?

We'll still have to hack around it in CheckPositionalParameterIndices, but...

What am I missing, in terms of complexity?

[syg]

I agree with @efaust. I think in the spirit of binary AST being a syntax tree, it's fine to keep duplicate entries in AssertedParameterScope. Since the PositionalParameterNames static semantics gathers all parameter names regardless of being duplicates or not, what hacks are needed for CheckPositionalParameterIndices?

[efaust]

@syg You have to keep track, when viewing the positional parameter at index 0, if you data structure says it's bound at index 1, that it's actually OK, because the index 1 exists.

A naive loop of "is this bound, and do the indices match" will fail you in this case.

[syg]

@efaust I see. But do you agree the spec is fine? For each asserted positional parameter name, it checks there's a matching actual positional parameter in both name and index, and that there are no unmatched parameters.

[arai-a]

If we just allow duplicate entry, the current spec works. (sorry, I thought that wasn't the case, thus I filed this issue)

Here's the trace of CheckParameterNames and CheckPositionalParameterIndices for function f(a, a, b) {}. both just pass. (to be clear, they don't check for duplication. in strict mode, SyntaxError is supposed to be thrown from not-BinAST-specific part of the spec)

3.1.4 CheckParameterNames ( expectedParams0, actualParams )
  // expectedParams0 = FrozenArray<AssertedMaybePositionalParameterName> = [
  //   AssertedPositionalParameterName {
  //     index: 0,
  //     name: "a",
  //     isCaptured: false,
  //   },
  //   AssertedPositionalParameterName {
  //     index: 1,
  //     name: "a",
  //     isCaptured: false,
  //   },
  //   AssertedPositionalParameterName {
  //     index: 2,
  //     name: "b",
  //     isCaptured: false,
  //   },
  // ]
  // actualParams = BoundNames of parameters = [
  //   "a",
  //   "a",
  //   "b",
  // ]
  1. Let expectedParams be a new empty List.
  2. For each p in expectedParams0, do
       a. Add p.name as the last element of expectedParams.
  // expectedParams = [
  //   "a",
  //   "a",
  //   "b",
  // ]
  3. Let idx be 0.
  4. If expectedParams and actualParams have different lengths, then throw a SyntaxError exception.
  5. For each pn in actualParams in List order, do
       // * idx = 0, pn = "a", expectedParams[idx] = "a",
       // * idx = 1, pn = "a", expectedParams[idx] = "a",
       // * idx = 2, pn = "b", expectedParams[idx] = "b",
       a. If expectedParams[idx] is not pn, then throw a SyntaxError exception.
       b. Set idx to idx + 1.

3.1.5 CheckPositionalParameterIndices ( expectedParams, positionalParamNames )
  // expectedParams0 = FrozenArray<AssertedMaybePositionalParameterName> = [
  //   AssertedPositionalParameterName {
  //     index: 0,
  //     name: "a",
  //     isCaptured: false,
  //   },
  //   AssertedPositionalParameterName {
  //     index: 1,
  //     name: "a",
  //     isCaptured: false,
  //   },
  //   AssertedPositionalParameterName {
  //     index: 2,
  //     name: "b",
  //     isCaptured: false,
  //   },
  // ]
  // positionalParamNames = PositionalParameterNames of parameters = [
  //   "a",
  //   "a",
  //   "b",
  // ]
  1. Let paramIndices be CreatePositionalParameterIndices(positionalParamNames).
  // paramIndices = [
  //   {
  //     [Name]: "a",
  //     [Index]: 0,
  //   },
  //   {
  //     [Name]: "a",
  //     [Index]: 1,
  //   },
  //   {
  //     [Name]: "b",
  //     [Index]: 2,
  //   },
  // ]
  2. For each p in expectedParams, do
       a. If p is an AssertedPositionalParameterName, then
            // * p = AssertedPositionalParameterName {
            //     index: 0,
            //     name: "a",
            //     isCaptured: false,
            //   },
            //   paramIndices contains {
            //     [Name]: "a",
            //     [Index]: 0,
            //   }
            // * p = AssertedPositionalParameterName {
            //     index: 1,
            //     name: "a",
            //     isCaptured: false,
            //   },
            //   paramIndices contains {
            //     [Name]: "a",
            //     [Index]: 1,
            //   }
            // * p = AssertedPositionalParameterName {
            //     index: 2,
            //     name: "b",
            //     isCaptured: false,
            //   },
            //   paramIndices contains {
            //     [Name]: "b",
            //     [Index]: 2,
            //   }
            i. If paramIndices contains an item item where item.[[Name]] is p.name and item.[[Index]] is p.index, then
                 1.Remove item from paramIndices.
           ii. Else,
                1. Throw a SyntaxError exception.
  // paramIndices = []
  3. If paramIndices is not empty, throw SyntaxError exception.

In that case, I'd suggest adding some note about duplication into those steps.