[Variadic Generics] require pack ident type within a pack expansion t…

[sophiapoirier]

…o form a pack reference

[hborla]

Woohoo, thank you! One minor piece of initial feedback - there are quite a few formatting changes, which make it difficult to spot the functional changes. Could you please either apply the formatting changes in a separate commit or drop them from this PR?

[xedin]

I suspect that clang-format has been ran for the whole file, you can use git clang-format instead and it would format only git added files before commit.

[sophiapoirier]

Yeah that was definitely not intentional, and I will figure out why it happened so I can stop it in the future, and revert that all here. Definitely none of that showed up locally when I did a confirming git diff --staged before committing.
I did follow the PR instructions which included running git clang-format after the commit, but when I did it printed that no files had changed. ¯\(ツ)/¯

[sophiapoirier]

I figured out the over-eager clang-formatting and fixed it, though what caused the cause remains mysterious.

[hborla]

I mentioned this offline, but I wonder if FromPackExpansion and AllowPackReferences can be one flag, as their uses seem to be very similar

[sophiapoirier]

Almost same... maybeResolvePackExpansionType (where the AllowPackReferences flag is added) can be called from resolvePackExpansionType (where the FromPackExpansion flag is added) but also from resolveASTFunctionTypeParams. AllowPackReferences is also added in a few other places outside of TypeCheckType.cpp
So running the tests in pack-expansion-expressions.swift, I see cases where resolvePackReference is reached and AllowPackReferences is set but FromPackExpansion is not. However in none of these cases is the new failure case logic in resolveDeclRefTypeRepr any different, because in every case of the flag mismatch, either isParameterPack and FromPackReference are both true or both false (the failure case is only when isParameterPack is true but FromPackReference is false).
That all said, I don't at this point have the insight into the several other places where the AllowPackReferences flag is added to know how they are reached and whether there could be a scenario where this new diagnostic could give a false negative?

[hborla]

Hmmm I wonder if that code in resolveASTFunctionTypeParams is now unnecessary / a remnant from when parameter packs used the same syntax as varargs parameters. I commented that code out and am running the tests now locally to see...

The other places where AllowPackReferences is set are all during constraint generation when we see an each T type repr inside an expression. All of those call-sites look like this:

      // Add a PackElementOf constraint for 'each T' type reprs.
      PackExpansionExpr *elementEnv = nullptr;
      if (!PackElementEnvironments.empty()) {
        options |= TypeResolutionFlags::AllowPackReferences;
        elementEnv = PackElementEnvironments.back();
      }

and !PackElementEnvironments.empty() is a guarantee that the each type repr appears inside of a PackExpansionExpr, so I don't think these places can cause any false negatives.

[hborla]

Oh! There's one other place AllowPackReferences is set in RequirementRequest because you can write each T in a generic requirement without repeat, e.g.

protocol P {}

func generic<each T>() where each T: P {}

...and I think we do want to diagnose something like where T: P!

[sophiapoirier]

Thank you @hborla for the further insights. I have examined and tested more and concluded that it will indeed be best to get rid of my new FromPackExpansion flag and just rely on AllowPackReferences.
I also found a couple of more tests that needed syntax fixing.

[hborla]

Looks great!

[sophiapoirier]

@swift-ci Please smoke test

[AnthonyLatsis]

This error message seems inaccurate. Is says an expansion cannot appear outside a function result, but syntactically the expansion is the function result.

[hborla]

I think "function result" should just be removed from the message. A previous experimental implementation of variadic generics that modeled packs as tuples (where this message was added) allowed returning a pack type T, but the current design does not allow for that. I also think we should eliminate the phrase "variadic expansion". I can update this message in a separate PR.