This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Handle duplicate enum constants in PopulateSwitch tweak
ClosedPublic

Authored by njames93 on Nov 1 2020, 5:54 AM.

Details

Summary

If an enum has different names for the same constant, make sure only the first one declared gets added into the switch. Failing to do so results in a compiler error as 2 case labels can't represent the same value.

enum Numbers{
One,
Un = One,
Two,
Deux = Two,
Three,
Trois = Three
};

// Old behaviour
switch (<Number>) {
  case One:
  case Un:
  case Two:
  case Duex:
  case Three:
  case Trois: break;
}

// New behaviour
switch (<Number>) {
  case One:
  case Two:
  case Three: break;
}

Diff Detail

Event Timeline

njames93 created this revision.Nov 1 2020, 5:54 AM
njames93 requested review of this revision.Nov 1 2020, 5:54 AM
njames93 updated this revision to Diff 302191.Nov 1 2020, 4:59 PM

Fix bug where prepare would return true erroneously when enum contains duplicated constants

Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2020, 4:59 PM

Thanks!
This makes sense to me, I assumed at first that the expr could be something complicated that we'd have to const-evaluate, thought clearly this already happens (and we do assume it's a ConstantExpr).

clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
115

It would still be nice to keep a high-level comment about the strategy.
(Not *this* comment of course, as this patch changes the strategy!)

121–122

why is the special case needed?

130

It looks as if we're now doing (via prepare + apply combined):

  1. Collect all constants into a SmallMapVector<APSInt,bool>.
  2. Iterate over cases, marking covered constants
  3. Prepare returns true if there are unmarked constants
  4. Collect all cases into a SmallSet<APSInt>
  5. Iterate over the constants, inserting text for those not in the set.

This seems equivalent, but less redundant:

  1. Collect all constants into a MapVector<APSInt, EnumConstantDecl*> (e.g. MissingEnumerators)
  2. Iterate over cases, deleting covered constants
  3. Prepare returns true if any constant remains
  4. Iterate over the map, inserting text for every element in the map
146–147

this comment no longer holds (we're not counting anymore).

Could just say // GNU range cases are rare, we don't support them

157

why can we not hit the APValue case?

185–188

Maybe just "Skip if value already covered (possibly under a different name)"?
("try to insert... if this fails" is just echoing the code)

njames93 updated this revision to Diff 303502.Nov 6 2020, 11:06 AM

Addressed comments.
Now using a MapVector to collect all uncovered cases in prepare, then just loop over that in apply.

njames93 marked 6 inline comments as done.Nov 6 2020, 11:11 AM
njames93 added inline comments.
clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
121–122

It wasn't was causing a bug early on then i fixed it and it was no longer needed

130

Thats a great suggestion. I did alter it slightly using a std::pair<EnumConstantDecl*, bool> for the value. This is because it would be nice to catch the case where the switch has a duplicated case value. The other reason is its linear time to call erase on a MapVector, so its potentially O(N^2) when looping through all the cases.

sammccall accepted this revision.Nov 6 2020, 2:53 PM
sammccall added inline comments.
clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
68

I'd make the values here a struct with named fields for readability, but up to you.

127

nit: I'd add a lambda here like

auto Normalize(llvm::APSInt Val) {
  Val = Val.extOrTrunc(EnumIntWidth);
  Val.setIsSigned(EnumIsSigned);
  return Val;
};

you'll use it twice, and the callsites will be more readable

129

it's already clear at this point.

165

this says if you ever have a case statement that doesn't match an enum value, then we don't offer the tweak. Why?

170

Do we really need to specially detect or handle this?

Seems like a blind

if (Iter != EnumConstants.end())
  Iter->second.second = true; // mark as covered

would be enough here

This revision is now accepted and ready to land.Nov 6 2020, 2:53 PM
njames93 marked 6 inline comments as done.Nov 6 2020, 3:58 PM
njames93 added inline comments.
clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
127

Good point.

129

I didn't know how tweaks exactly worked, like if the tweak object is instantiated for each selection or if its reused. I'll remove this.

165

If the user puts in a case statement that doesn't directly match a value, chances are they are doing something funky. In which case the tweak is likely not going to help. WDYT?

170

What are the rules for clangd tweaks on ill-formed code. I'll remove this anyhow.

njames93 updated this revision to Diff 303580.Nov 6 2020, 3:59 PM
njames93 marked 3 inline comments as done.

Address comments

sammccall accepted this revision.Nov 9 2020, 3:01 AM

Fantastic, thanks!

Still LG, comments optional.

clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
62

for this kind of purpose, we'd typically just use struct EnumAndCovered { const ECD *Enumerator; bool Covered; } without encapsulation. But this is fine.

I think a name like ExpectedCase might hint a little more at the purpose. Up to you.

165

I agree it signals that exhaustively enumerating the cases *might* not be useful. (It still could be though, e.g. imagine an enum that can't easily be extended where someone's added a sentinel value).

Since this is a code action rather than a diagnostic, I'd lean towards still offering the action, but either seems reasonable.
If you're going to bail out in this case, it definitely deserves a comment.

170

Basically, don't crash on ill-formed code, try not to produce wildly invalid syntax.
Beyond that, we only really care about problems that come up a lot in practice.

(That said, if there are duplicated cases, I'm not sure that should stop us applying this tweak)

This revision was landed with ongoing or failed builds.Nov 9 2020, 4:15 AM
This revision was automatically updated to reflect the committed changes.