[clang] [Clang] Address feedback in PR183010 (PR #185608)

[Matheus Izvekov via cfe-commits]

================
@@ -1185,7 +1154,7 @@ static bool CheckConstraintSatisfaction(
     return false;
   }
 
-  if (TemplateArgsLists.isAnyArgInstantiationDependent(S.Context)) {
+  if (TemplateArgsLists.isAnyArgDependent(S.Context)) {
----------------
mizvekov wrote:

> Maybe just `C.getCanonicalTemplateArgument(TA).isDependent()` (which would works for expressions, expression canonicalization just returns the expression).

For a plain "isDependent" check, `getCanonicalTemplateArgument` buys you nothing, as an AST node will be dependent 'iff' the corresponding canonical node is dependent. It costs performance for no benefit, because canonicalizing TemplateNames is not cheap.

> But it is possible I misunderstood. In particular this does not address Matheus' examples.
But I think these examples are unrelated as they are incorrectly compiled with or without the patches Younan has been working on.

There is a misunderstanding here, #183010 does cause the regression from the second example (where the concept template parameter is used).

That's not surprising, I am not presenting a random example here for no reason, this example came up from analyzing the changes and thinking about the consequences of it.

The change in #183010, even including the changes from this PR, are incorrect because of the reasons I already explained before, the existing code was already doing the right thing.

The cache is not the cause here, and it's not a general problem with 'sugars', the problem is fundamentally that you cannot perform constraint satisfaction checking if any of the arguments involved contain unsubstituted template parameters. That's because you don't know if the types involved will become invalid.
This is specially relevant for concepts because they trap substitution failures and turn them into a 'false' result.
And for better or for worse, that's part of their design intent. 

And that's exactly why the original code was like that, it was deferring satisfaction checking if any template arguments were instantiation dependent (ie they contain unsubstituted template parameters, even if that doesn't change clang's idea of canonical types).

The patch changes that without really explaining what it's doing. From looking at the patch, it 'gives the impression' that the original code was wrong all along.

I think the main objections here are:
* We must be careful to document where we have made tradeoffs and workarounds, to the best of our knowledge. This includes putting the FIXMES in the right places (ie in the lines changed in #183010, not blaming the cache).
* We have a policy to avoid backports which introduce new regressions. I'd escalate if we want to make an exception here. Certainly at least the people involved in managing the stable release should not be left in the dark about this.

https://github.com/llvm/llvm-project/pull/185608