RD won't allow a rename of an enum member with otherwise illegal characters.

[ThunderFrame]

Standard naming rules don't apply to enum members (or enums themselves), this code is perfectly legal (but might be worthy of an inspection). At the very least, a leading underscore should be permitted in order to take advantage of the hidden enum member feature.

Enum Foo
  Apple = 1
  [_Min] = 1
  [`~!@#$%^&*()-_+=[']\/?{}|<>,.;:"] = 3
End Enum

A more difficult issue relates to the name of an enum, as per #2568, where special characters are also allowed, but the VBE pretty printer fails to add/retain the necessary square brackets:

Enum [`~!@#$%^&*()-_+=[']\/?{}|<>,.;:"]
  Apple = 1
  [_Min] = 1
  [`~!@#$%^&*()-_+=[']\/?{}|<   spaces!   >,.;:"] = 3
End Enum

'pretty print to become:

Enum `~!@#$%^&*()-_+=[']\/?{}|<>,.;:"
  Apple = 1
  [_Min] = 1
  [`~!@#$%^&*()-_+=[']\/?{}|<   spaces!   >,.;:"] = 3
End Enum

[Vogel612]

We really need a "WTF VBA?" tag

[rubberduck203]

Don't tempt me. We can make that happen.

[ThunderFrame]

@rubberduck203 it sounds like you're prioritizing the tag over the fix.. :sad:

[rubberduck203]

Well, I'm really just moral support at this point, but I do still have the power to create funny labels.

[daFreeMan]

@ThunderFrame I pray that you're the only person on the planet who thinks of these things and that's why they're untested in the VBE.

What kind of bizarre, twisted thoughts would be going through a programmer's head if he was using names like that in actual production code???

[rubberduck203]

And that ^ is precisely why RD should fix the bug.

[retailcoder]

Possibly related: #3238

[bclothier]

I'm inclined to think this should be closed because I just don't see the merits of enabling users to do stuff that nobody will want to do. Even if we did fix up the ducky to handle those horrid names, it doesn't seem sensible to support refactoring toward worse code. We want them to refactor away from bad code, no?

[mansellan]

Not sure... wouldn't the ideal be that the ducky can eat the bad granola, then explain why it's terrible code with some pithy inspections? Allow legal, suggest sensible?

[MDoerner]

It would be ideal indeed. Unfortunately, this is a really hard problem because it does not fit to the lexer rules at all. (I think, if the VBE did not remove the square brackets, RD could already handle this situation correctly.)

[mansellan]

IIUC the OP correctly though, that applies only to enum names, not enum member names. Does this need splitting out into separate issues?

EDIT: #2568 already covers enum names, I guess that's a better candidate for closure?

[rubberduck203]

Allow legal, suggest sensible?

I think this is a great take on the old adage “Make the common things easy and the hard things possible.”

“Allow legal. Suggest sensible.” is a very good design goal for this project IMO.

[retailcoder]

Hm, I read it as it's both.

The general guiding idea behind RD's parser, is that we want any code that is legal VBA code to parse correctly (no matter how horrible).

My understanding of the issue is that we do correctly parse them (save for the bracketless enum name), but then the rename refactoring is refusing to accept such an identifier for a new name. If that is correct, then I agree with the OP, that we should at least allow a leading underscore (and apply the square brackets accordingly) if the target is an enum member.

If the target is the enum itself, then tough luck.

[bclothier]

I agree with the idea of allowing underscore which does have a good use case. It's the unrestricted filter that I can't bring myself to agree with.

[mansellan]

linking chat

How about we add a "hint" to the dialog:

• rename a member to leading underscore: icon-info: This will make the member hidden (add square brackets)
• rename a member to weirdness: icon-warning: Inadvisable (add square brackets)
• rename an enum to weirdness: icon-error: This will not survive edits (disallow)

[retailcoder]

I think this should simply be handled by the validator - UI cues are nice, but won't affect the identifier logic.

[bclothier]

That is true but validator should provide a mechanism to communicate the issues for the corresponding UI cues to be displayed. Perhaps some kind of IdentifierValidationResult struct or something like that.

[BZngr]

FWIW: The recently merged VBAIdentifierValidator has: bool TryMatchInvalidIdentifierCriteria(string name, DeclarationType declarationType, out string criteriaMatchMessage) to provide a reason for the invalid result - so something can be provided to the UI if needed.

[retailcoder]

@BZngr that's a great start, we we just chatting about this! We were saying maybe returning a IdentifierValidationResult struct that would include a bool IsValid and an enum value pointing to a reason for being invalid - and then the UI would be responsible for pulling the appropriate resource string for it.

[retailcoder]

FWIW I don't think differentiating "illegal" from "legal but write-once and cannot be edited" is worthwhile.

public enum IdentifierValidationInfo
{
    Accepted, // no issues with the name
    IllegalIdentifier, // name is not legal
    BadIdentifier, // name is legal, but may trigger other warnings
    HiddenEnumMember, // name has underscore prefix and is for an enum member

}

[BZngr]

Just to throw this out there...when writing the validator code, I thought it would have be nice to somehow attach/associate the validation results to the Declaration.Identifier property but didn't pursue the idea for the PR.

[bclothier]

That would be basically an inspection, in fact.

[BZngr]

Given the following:

Public Enum FruitType
    [_First] = 1
    Apple = 1
    Orange = 2
    Plum = 3
    [_Last] = 3
End Enum

Sub DoSomething()
    MsgBox CStr(FruitType.[_First])
End Sub

The References collection for Enum Member [_First] is empty. It does not recognize MsgBox CStr(FruitType.[_First]) as a reference. This issue is described further in #5228