Skip to content
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

These should probably not be possible #10165

Closed
KirkMunro opened this issue Jul 15, 2019 · 8 comments
Closed

These should probably not be possible #10165

KirkMunro opened this issue Jul 15, 2019 · 8 comments
Labels
Breaking-Change breaking change that may affect users Committee-Reviewed PS-Committee has reviewed this and made a decision Issue-Question ideally support can be provided via other mechanisms, but sometimes folks do open an issue to get a Resolution-Answered The question is answered. WG-Language parser, language semantics

Comments

@KirkMunro
Copy link
Contributor

Steps to reproduce

# Create a variable name that is nothing but spaces:
${ } = 'boo'
${ }

# Create a variable name that contains leading or trailing whitespace:
${   foo   } = 'bar'
$foo # Not found
Get-Variable -Name '   foo   ' -ValueOnly

# Create a function whose name contains leading whitespace (trailing whitespace is silently stripped)
New-Item 'function: foo ' -Value {'Whitespace only'}
& ' foo ' # Not found
& ' foo' # This works (the trailing whitespace was ignored during creation of the function)

# Create an alias whose name is nothing but whitespace
New-Alias -Name ' ' -Value Get-Date
& ' ' # Shows the date

# Set a breakpoint on a command that is just whitespace
Set-PSBreakpoint -Command ' '

# Try to trigger a breakpoint on that command
& ' ' # Does not work

# Set a breakpoint on a variable that is just whitespace
Set-PSBreakpoint -Variable ' ' -Mode ReadWrite

# Try to trigger a breakpoint on a variable whose name is nothing but whitespace
${ } # triggers breakpoint

# Create a filename in the current location that has leading and trailing whitespace in its name
New-Item ' huh.txt ' -Value 'Explorer does not allow such names to be created'

# Open the file in notepad
notepad ' huh.txt ' # works
notepad ' huh.txt' # also works even though the trailing whitespace was dropped
notepad 'huh.txt' # does not work; prompts to create a new file

Expected behavior

  1. Command and variable names should either be trimmed so that they contain no extraneous whitespace, or they should raise an error if they contain extraneous whitespace so that there is no ambiguity on the part of the person creating the command or variable.

  2. Set-PSBreakpoint should trim whitespace from string parameters.

Actual behavior

As indicated in the comments above.

How we might fix this

Define an AutoTrim attribute

This attribute would automatically trim string values before they are validated against validation attributes. That corrects the contents for commands, which is helpful; however, it means users can pass in values and get results that are different than those values (i.e. create an alias with leading/trailing whitespace and end up with alias that doesn't have the whitespace). This could hide bugs that exist in calling code.

Define a ValidateNoExtraneousWhitespace attribute

This attribute would result in an error in the parameter binder if a parameter of type string or a parameter that is a collection of type string contained strings that included leading or trailing whitespace, forcing the caller to deal with the problem. This is my preferred partial solution to this issue.

Questions

Is there anything we can/should do about provider paths with leading/trailing whitespace? For PowerShell entities such as functions/aliases/variables, we can obviously generate errors if leading/trailing whitespace is used, but since files exist outside of PowerShell and can be created with spaces in Windows (not sure about macOS/Linux, haven't tested there), we probably can't provide a generic solution that prevents use of paths with extraneous whitespace.

Environment data

Name                           Value
----                           -----
PSVersion                      6.2.1
PSEdition                      Core
GitCommitId                    6.2.1
OS                             Microsoft Windows 10.0.17763
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0
@KirkMunro KirkMunro added the Issue-Question ideally support can be provided via other mechanisms, but sometimes folks do open an issue to get a label Jul 15, 2019
@iSazonov iSazonov added the WG-Language parser, language semantics label Jul 16, 2019
@iSazonov
Copy link
Collaborator

Is there anything we can/should do about provider paths with leading/trailing whitespace?

For FileSystem provider. Trailing whitespaces is removed. Leading whitespaces is meaningful.

@iSazonov
Copy link
Collaborator

/cc @mklement0 For information.

@iSazonov
Copy link
Collaborator

iSazonov commented Jul 23, 2019

/cc @SteveL-MSFT
For PowerShell Committee review.
From the issue and #10148, #10157

How we should resolve names (variables, aliases, functions/cmdlets) with whitespace characters (space, tab)?

  • at start like " abc"
  • at end like "abc "
  • with whitespace only like " "

Suggestions is

  • to disable with ArgumentException at creation time and CommandNotFoundException
    /VariableNotFoundException/AliasNotFoundException at getting time.
  • to trim silently.

@iSazonov iSazonov added Breaking-Change breaking change that may affect users Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jul 23, 2019
@JamesWTruher
Copy link
Member

Why are we considering that we own the mandate for how people consider naming their variables. I really don't think there's a problem here. If you don't like this, then don't use it, but please don't try to tell me what I can name my variables.

@KirkMunro
Copy link
Contributor Author

@JamesWTruher The problem is that bugs and unexpected behavior shows up from these types of edge cases. It isn't about telling you what you can/cannot use for variable/function/alias names. It is about ensuring there is some expectation of order though, so that you don't run into obscure bugs or unexpected behavior that show up when you use names that are not expected. Such "restrictions" would help the scripter, not just in their use of PowerShell, but in being a scripter/developer in general.

On the flip side, if we don't do this, do we have tests in place for such things (variable/function/alias names that are whitespace only or that start and/or end with whitespace), so that the scripter can reasonably expect them to work just fine? I'd wager that we don't, and I think it would be better to guide PowerShell scripter here in the right direction than to spend time testing/fixing the engine so that it properly supports such things throughout the language.

A few well-intentioned limits here would be a good thing in my book.

@KirkMunro
Copy link
Contributor Author

KirkMunro commented Jul 25, 2019

Here's another example that came up in an email list today, where appropriate boundaries are not in place and what may appear as strange bugs show up as a result:

# You can define a function with a parameter name that is numeric.
# This should not be allowed.
function Test-ParameterName {
    [CmdletBinding()]
    param([Switch]$7)
    # The body doesn't matter
}

# You can define a function with a parameter alias that is numeric.
function Test-ParameterAlias {
    [CmdletBinding()]
    param([Alias('7')][Switch]$NumericAliasTest)
    # The body doesn't matter
}

# You can't invoke either of these function with a parameter name or
# alias that is numeric.
# Strange bugs show up when you don't put appropriate boundaries
# in place around command/variable/etc. names
Test-ParameterName -7 # Returns an error
Test-ParameterAlias -7 # Returns an error

In this case, we should have rules for parameter names and parameter aliases. For example, if they start with a number, they must contain at least one alphabetic character. But even then, there are other considerations that need to be taken into account, like this one:

function Test-ParameterName {
    [CmdletBinding()]
    param([Switch]$10mb)
    # The body doesn't matter
}

# You can't invoke this with the parameter either because the name is
# still numeric, even though it contains alphabetic characters.
Test-ParameterName -10mb # Returns an error

I think we should support parameter names that are numeric, so that a command could have a parameter name such as -32bit, for example, but more consideration should be given to the many different types of numeric constants that can be used that contain alphabetic characters (e.g. -1gb, -1d, -1e10, -0xabcd, etc). In the case where a parameter name or a parameter alias starts with one or more numbers, maybe the parser should prevent any scenarios that represent static numbers. They aren't likely to come up as desirable parameter names anyway, perhaps with the exception of -0xbadf00d...😛

@SteveL-MSFT
Copy link
Member

@PowerShell/powershell-committee reviewed this, we don't see the value to spend time addressing this as this has been existing behavior forever and hasn't been an issue. Filenames allow whitespace and this follows that convention. Users can choose to use this if they choose and we shouldn't break them.

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision Resolution-Answered The question is answered. and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jul 31, 2019
@msftrncs
Copy link
Contributor

@KirkMunro, you can use parameters that start like -32bit, just not via the - designator, they instead require using hashes and the splat operator, or using an invoke object with parameter hash and with a new method coming soon with @@{'32bit'=$true}.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking-Change breaking change that may affect users Committee-Reviewed PS-Committee has reviewed this and made a decision Issue-Question ideally support can be provided via other mechanisms, but sometimes folks do open an issue to get a Resolution-Answered The question is answered. WG-Language parser, language semantics
Projects
None yet
Development

No branches or pull requests

5 participants