Page MenuHomePhabricator

[clang] Improve diagnostics for expansion length mismatch
ClosedPublic

Authored by mizvekov on Jun 17 2022, 1:39 PM.

Details

Summary

When checking parameter packs for expansion, instead of basing the diagnostic for
length mismatch for outer parameters only on the known number of expansions,
we should also analyze SubstTemplateTypeParmPackType and SubstNonTypeTemplateParmPackExpr
for unexpanded packs, so we can emit a diagnostic pointing to a concrete
outer parameter.

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Diff Detail

Event Timeline

mizvekov created this revision.Jun 17 2022, 1:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2022, 1:39 PM
mizvekov updated this revision to Diff 438073.Jun 17 2022, 5:19 PM
mizvekov retitled this revision from clang: fix pack substitution scope in function params to clang: fix checking parameter packs for expansion..
mizvekov edited the summary of this revision. (Show Details)

.

mizvekov retitled this revision from clang: fix checking parameter packs for expansion. to clang: fix checking parameter packs for expansion.Jun 17 2022, 5:20 PM
mizvekov published this revision for review.Jun 17 2022, 5:47 PM
mizvekov added reviewers: rsmith, v.g.vassilev.
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2022, 5:47 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
mizvekov updated this revision to Diff 438680.Jun 21 2022, 6:41 AM
mizvekov edited the summary of this revision. (Show Details)

.

mizvekov updated this revision to Diff 445369.Jul 17 2022, 6:43 PM
mizvekov edited the summary of this revision. (Show Details)
mizvekov updated this revision to Diff 447995.Jul 27 2022, 3:56 AM
mizvekov added reviewers: Restricted Project, aaron.ballman, erichkeane, shafik.Aug 10 2022, 3:26 AM

Can you make a better commit message? I don't have a good idea of the intent/description of what you're trying to do here.

mizvekov updated this revision to Diff 452306.Aug 12 2022, 2:34 PM
mizvekov retitled this revision from clang: fix checking parameter packs for expansion to [clang] Improve diagnostics for expansion length mismatch.
mizvekov edited the summary of this revision. (Show Details)

Can you make a better commit message? I don't have a good idea of the intent/description of what you're trying to do here.

Done.

I split this up into two patches:

  • D131802: Introduces the test and a simple fix.
  • This one which improves the analyzer to give better diagnostics.
mizvekov updated this revision to Diff 452693.Aug 15 2022, 8:50 AM
mizvekov edited the summary of this revision. (Show Details)
shafik added inline comments.Aug 15 2022, 2:54 PM
clang/lib/Sema/SemaTemplateDeduction.cpp
741–745

Structured bindings maybe?

mizvekov added inline comments.Aug 15 2022, 3:09 PM
clang/lib/Sema/SemaTemplateDeduction.cpp
741–745

Sure, they weren't an option back when this patch was first submitted :)

mizvekov updated this revision to Diff 453757.Aug 18 2022, 12:39 PM
mizvekov marked an inline comment as done.Aug 18 2022, 12:39 PM
aaron.ballman added inline comments.Aug 29 2022, 5:36 AM
clang/lib/Sema/SemaTemplateVariadic.cpp
103–106

Do we need to handle FunctionParmPackExpr as well?

mizvekov added inline comments.Aug 29 2022, 5:48 AM
clang/lib/Sema/SemaTemplateVariadic.cpp
103–106

Right, that and SubstTemplateTemplateParmPack are the two missing cases we could handle here, and perhaps that would allow us to get rid of that 'from outer parameter packs' diagnostics completely.

Would you prefer to handle everything in one patch?

just a pair of minor changes I'd like to see, otherwise this LGTM.

clang/lib/Sema/SemaTemplateVariadic.cpp
103–106

I think I'm alright doing those in a followup patch.

699

Can you please make this an else if at the level higher? The 'else' after this ends up being so small that it gets lost trying to figure out what is going on here.

So:

} else if (const auto *VD = dyn_cast<VarDecl>(ParmPack.first.get<const NamedDecl *>()))

844

same comment here, see if the 'vardecl' logic can be moved up a level in the 'if' tree.

aaron.ballman added inline comments.Aug 29 2022, 6:14 AM
clang/lib/Sema/SemaTemplateVariadic.cpp
103–106

I'm also fine handling it in a follow-up if the changes are involved.

mizvekov planned changes to this revision.Aug 29 2022, 4:36 PM
mizvekov updated this revision to Diff 456494.
mizvekov retitled this revision from [clang] Improve diagnostics for expansion length mismatch to WIP: [clang] Improve diagnostics for expansion length mismatch.
mizvekov requested review of this revision.Aug 29 2022, 4:39 PM
mizvekov updated this revision to Diff 456495.
mizvekov retitled this revision from WIP: [clang] Improve diagnostics for expansion length mismatch to [clang] Improve diagnostics for expansion length mismatch.
mizvekov marked 5 inline comments as done.
erichkeane added inline comments.Aug 29 2022, 4:53 PM
clang/lib/Sema/SemaTemplateVariadic.cpp
845

This pattern with the init + condition is a little awkward here... any reason to not just use the cast around the 'ND" and just use the VD in the use below? or is there a good reason to split it up like this?

Same with the above case.

mizvekov added inline comments.Aug 29 2022, 5:12 PM
clang/lib/Sema/SemaTemplateVariadic.cpp
845

No very strong reason than that just from my different perception this looked fine :)

Minor advantage that we don't make the variable a VarDecl pointer if we don't need to access it as such.

But no strong preference here, I can have another look tomorrow.

mizvekov updated this revision to Diff 456682.Aug 30 2022, 8:17 AM
mizvekov added inline comments.Aug 30 2022, 8:18 AM
clang/lib/Sema/SemaTemplateVariadic.cpp
845

I played a little bit with this change.

I think one thing that makes that pattern of adding separate ND and VD a bit confusing is that at a glance it almost looks like these are different cases in the PointerUnion variant. You have to do a double take to see that, since the nested cast is a bit subtle. This can become even subtler as we add more cases in the next patch.

Or we could stop using PointerUnion on the next patch, since it's so unergonomic with so many variants.

Anyway, I did some other refactorings and I think in general this will be much clearer to read on my next push.

With this refactoring, on this part here that problem goes away since we make this section produce a NamedDecl.

On the second part, below, I tidied up the code so much that I think that nested else isn't almost invisible anymore, since the other blocks become about the same size.

Otherwise, let me know what you think.

I also added to this first part here a FIXME comment describing a pre-existing problem where if we get a canonical TTP, the diagnostics fail to point to the relevant parameter since we won't have it's identifier.

Will try to add a repro and fix for that on the next patch.

erichkeane accepted this revision.Aug 30 2022, 8:25 AM

Happy enough, LGTM.

clang/lib/Sema/SemaTemplateVariadic.cpp
845

Thanks for spending time on this... the nested conditionals on the var-decl was hiding 90%+ of what was going on in the branch, and at least this top one is BETTER enough than before. I too hate the pointer union (note BTW, that _4_ is the maximum number of elements thanks to 32 bit platforms, that'll burn you :)).

This revision is now accepted and ready to land.Aug 30 2022, 8:25 AM
mizvekov marked 2 inline comments as done.Aug 30 2022, 10:01 AM
mizvekov added inline comments.
clang/lib/Sema/SemaTemplateVariadic.cpp
845

Thanks, true about the maximum size :)

Some of the cases could be unified as we have two types and two expressions.