New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

`switch` statement: prevent nonsensical / repeated options #8598

Open
mklement0 opened this Issue Jan 7, 2019 · 6 comments

Comments

Projects
None yet
3 participants
@mklement0
Copy link
Contributor

mklement0 commented Jan 7, 2019

Summary of the new feature/enhancement

While the precedence rules that apply when incompatible / repeated options are passed to the switch statements are documented (partially incorrectly), a user-friendlier option would be to prevent such combinations to begin with.

Notably, the following combinations are currently permitted, with the last option winning:

  • Any combination of conceptually mutually exclusive options -exact, -wildcard, and -regex

  • Multiple -file <file> options.

  • Repeated use of the same option.

Proposed technical implementation details (optional)

Report a parse-time error when such pointless combinations are found.

Technically, that would be a breaking change, but, I hope, of type Bucket 3: Unlikely Grey Area.

@vexx32

This comment has been minimized.

Copy link
Contributor

vexx32 commented Jan 7, 2019

This should be relatively straightforward, I should think. Probably have to pick it up in the tokenizer, or potentially the parser if that turns out to be a simpler point to detect it at (as it well might, it having been already neatly chopped into tokens by that point).

Are there any other keywords that take parameters like this? I can't think of any off the top of my head, but if there are we probably ought to check we don't have a similar error there.

@mklement0

This comment has been minimized.

Copy link
Contributor

mklement0 commented Jan 7, 2019

I'm glad to hear it, @vexx32.

From what I can glean from about_language_keywords, the only other statement that supports options with the same type of syntax is DATA, which supports only the -SupportedCommand option, which, however, is already parsed as expected: it should and does permit only one instance of -SupportedCommand (to which an array of command names may be passed).

@vexx32

This comment has been minimized.

Copy link
Contributor

vexx32 commented Jan 7, 2019

I have never even heard that mentioned in passing. Ever. Didn't even know that was a language keyword, and I've read that help doc several times.... wow. Must have just skimmed over it a good many times.

That needs a thorough write up... guess I know what I'm doing for next week's blog, haha! That is really nice to know about, thank you!

@BrucePay

This comment has been minimized.

Copy link
Member

BrucePay commented Jan 7, 2019

@mklement0

Any combination of conceptually mutually exclusive options -exact, -wildcard, and -regex

But they are not actually mutually exclusive. You can specify multiple options and the last one wins (i.e. order matters). So this is definitely a breaking change. What benefit would we accrue from taking this breaking change?

From a historical perspective, IIRC, in V1, there might have been some thought that these would be switch parameters but because they are language elements instead of parameters, that doesn't work. @JamesWTruher @khansen00 - do you guys remember any more details about this?

@vexx32 The code is here:

Parser.cs:2559

@mklement0

From what I can glean from about_language_keywords, the only other statement that supports options with the same type of syntax is DATA,

The DSC configuration keyword also takes 'parameters' however as a general rule, we tried to avoid parameters on language elements because they don't behave like proper cmdlet parameters.

@vexx32

This comment has been minimized.

Copy link
Contributor

vexx32 commented Jan 7, 2019

They're functionally mutually exclusive, though. You can only use one functionality at a time, which makes the suggestions in the current documentation confusing at best, in my opinion.

Would be interesting to hear any further background on this!

@mklement0

This comment has been minimized.

Copy link
Contributor

mklement0 commented Jan 8, 2019

To add to @vexx32's comment:

That they're not actually mutually exclusive is the very reason for creating this issue: they should be, which brings us to why this change is worth making:

If we prevent nonsensical / pointless use to begin with, we reduce complexity in several respects:

  • Users are spared from accidentally creating self-contradictory / confusing switch statements.
  • Similarly, users reading existing code won't be faced with such statements.
  • There is no need to establish and document precedence rules, whose sole raison d'être is to resolve ambiguity due to self-contradictory / repeated-options use.
  • Users therefore need not be aware of and need not learn these precedence rules.

As for what could break:

  • Contradictory use: Statements such as switch -wildcard -regex ...

    • Clearly, no one should ever have written such a statement, as it betrays a lack of understanding. (Note that programmatic construction of the options (via splatting) doesn't apply to switch, leaving Invoke-Expression aside, which I don't think we need to worry about).
  • Pointless repetition: Statements such as switch -wildcard -wildcard ... and switch -file 1.txt -file 2.txt ...

    • Such repetition is benign with the parameterless options, but problematic with -file (again, the last one wins) - if there really is concern about breaking such usage, we could limit the change to preventing contradictory use.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment