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

#Requires doesn't require separation #9097

Closed
msftrncs opened this issue Mar 9, 2019 · 11 comments
Closed

#Requires doesn't require separation #9097

msftrncs opened this issue Mar 9, 2019 · 11 comments
Labels
Issue-Question ideally support can be provided via other mechanisms, but sometimes folks do open an issue to get a Resolution-No Activity Issue has had no activity for 6 months or more WG-Engine core PowerShell engine, interpreter, and runtime

Comments

@msftrncs
Copy link
Contributor

msftrncs commented Mar 9, 2019

Steps to reproduce

#requires-modules psworkflow
#requiresCrazyThings -psed desktop

The #Requires directive doesn't seem to require some kind of separation from its keyword to the next token, but not providing that separation causes an error anyway.

PS C:\> #requiresCrazyThings -psed desktop
At line:1 char:2
+ #requiresCrazyThings -psed desktop
+  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Cannot process the #requires statement because it is not in the correct format.
The #requires statement must be in one of the following formats:
 "#requires -shellid <shellID>"
 "#requires -version <major.minor>"
 "#requires -psedition <edition>"
 "#requires -pssnapin <psSnapInName> [-version <major.minor>]"
 "#requires -modules <ModuleSpecification>"
 "#requires -runasadministrator"
+ CategoryInfo          : ParserError: (:) [], ParentContainsErrorRecordException
+ FullyQualifiedErrorId : ScriptRequiresInvalidFormat

PS C:\>

I am just not sure this is right. A inexperienced user could stumble across this by accident thinking he's just entering a comment. I have seen plenty of reports already of new users who are confused when the editor starts highlighting items in a comment, turning them in as bug reports.

@msftrncs msftrncs added the Issue-Question ideally support can be provided via other mechanisms, but sometimes folks do open an issue to get a label Mar 9, 2019
@msftrncs msftrncs changed the title #Requires doesn't require seperation #Requires doesn't require separation Mar 9, 2019
@kvprasoon
Copy link
Contributor

Isn't the error message helpful ?

@mklement0
Copy link
Contributor

More fundamentally, the issue is that it is problematic that the syntactic form of a comment was chosen to implement "preprocessor directives" (statements evaluated at script/module parse time), which invites confusion.

I suggest either reframing this issue accordingly or creating a new issue to suggest a distinct syntax form.

@msftrncs
Copy link
Contributor Author

I just think that #requires should be held to the same standard as any other keyword, it must adhere to delimiter standards to be acknowledged. #requiresCrazyThings does not meet the delimiter requires anywhere else in PowerShell for a keyword of #requires. Everywhere else, the token would be #requiresCrazyThings and that is not the same as #requires. Because it doesn't match the required special preprocessor directive keyword, the line should be ignored, not produce an error for an 'almost' attempt.

I do realize that this is a case of error one way or the other, but I think because its based on a comment marker, the error should be in the direction of ignoring anything that isn't exactly right. Its a comment, and instead let the user figure out why his very badly malformed #requires directive isn't working. If the keyword token has met the standard delimiter and the rest of the syntax is in error, then display an error.

Background, I am working on a PR over at PowerShell/EditorSyntax, PowerShell/EditorSyntax#156, and trying to make sure it mostly matches what PowerShell really expects.

@mklement0
Copy link
Contributor

I do realize that this is a case of error one way or the other, but I think because its based on a comment marker, the error should be in the direction of ignoring anything that isn't exactly right.

On purely technical grounds I'd agree: if something isn't a syntactically valid #requires directive, it shouldn't be treated as one.

However, falling back to the other construct that shares the #-prefixed syntax - a comment - means that what the user may have intended to be a #requires directive will be quietly ignored.

My sense is that this quiet ignoring would be worse than the current behavior, as a script author may not notice that a directive they meant to put in place wasn't.

If they didn't mean to create a #requires directive and #requiresCrazyThings was actually meant to be a comment, then at least the error message will alert them to how their comment is being interpreted as something else - and working around the problem - should it ever arise - is trivial: simply put a space between # and requires.

As simple as that workaround is - and if we stick with the current behavior the error message should be amended to suggest it - it highlights the fundamental problem:

Comment syntax should never have been overloaded to serve as directive syntax too, and a proper solution requires a distinct syntax form for directives.

@vexx32
Copy link
Collaborator

vexx32 commented Mar 11, 2019

In general I tend to lean towards always having a leading space on my comments following the # symbol, so I guess I've never run into this as an issue. I agree, though, that using the standard comment syntax as a directive is probably not the best.

I'm sure it was intended to reflect back on C#'s preprocessor-directive syntax, but that might have been a bad idea.

Not sure if it can really be changed now, though? I guess it wouldn't be the end of the world, as changing it would essentially mean that all current #Requires statements are just... comments. I guess it'd be OK?

Not sure what we could really use instead. Any thoughts on viable alternatives @mklement0? 🤔

@mklement0
Copy link
Contributor

The using directives already are bona fide statements (though, unlike the #requires directives, they must be at the top of the script).

using module even more or less does the same thing as #requires -module (the former additionally loads PS classes).

using isn't really the right name for the other #requires directives (using runasadministrator?), however, so perhaps a separate requires keyword is called for.

I'm pretty sure that an RFC is needed to propose something of that magnitude.

In the current situation, my recommendation is to merely amend the error message so that if someone writes #requiresCrazyThings or even #requires time to process (where #requires is technically a valid keyword), they'll see something along the lines of "If you meant to write a comment instead, put a space between '#' and the word 'requires'" after the current message.

@msftrncs
Copy link
Contributor Author

A couple thoughts:

  • The reasoning for using a comment for a directive was probably because of the low usage of the keyword. In most languages, a keyword must be reserved in its entirety, and in this case, to only one purpose. Using a comment marker eliminates that need to reserve the word from any other use.
  • A second reason, is that, new concepts can be added without breaking backwards compatibility when they serve minor value. Not saying that's what is going on here, but another valid reason they exist. Thinking of it that way, comment based directives should really only be considered minor, which is not really what requires does, except that if the requires directive is skipped, eventually an error will occur. Its just that by then, the damage might already have been done. A properly written program would have damage control. In this case, requires could be just a easy way out of writing damage control.
  • My main concern, is that there is a lack of a requirement for a delimiter between the keyword and its arguments, and further agitated that the example of a seemingly valid construct for an argument is not accepted without a delimiter, when in fact one is not required for the keyword itself. ie #requires-modules psworkflow still returns an error, while anywhere else in PowerShell, the token would been decoded as requires-modules and that wouldn't match requires. I unfortunately do not have enough experience with other languages that do this (commented directives) to know how they behave, but the behavior seems odd.
  • usings requirement to be the first line(s) is solution to the reservation of a keyword, albeit with its own restrictions. using namespace could just as well as been a comment directive, as its not critical (it just makes the writer type longer name spaces).

In actuality, most of the work of using or #requires can be accomplished directly in the scripting language, where more specific error messages could be crafted, or even the issue worked around, maybe with the exception of importing classes from a module.

I can implement the current syntax in the tmlanguage grammar, it will just be the odd one out, in all the rules, and is why I bring this up. I think its better to have a good understanding of the grammar than to just assume it works a certain way, hence my investigation in to it in the first place.

@iSazonov iSazonov added the WG-Engine core PowerShell engine, interpreter, and runtime label Jan 15, 2021
Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

2 similar comments
Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

@microsoft-github-policy-service microsoft-github-policy-service bot added Resolution-No Activity Issue has had no activity for 6 months or more labels Nov 16, 2023
Copy link
Contributor

This issue has been marked as "No Activity" as there has been no activity for 6 months. It has been closed for housekeeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Question ideally support can be provided via other mechanisms, but sometimes folks do open an issue to get a Resolution-No Activity Issue has had no activity for 6 months or more WG-Engine core PowerShell engine, interpreter, and runtime
Projects
None yet
Development

No branches or pull requests

5 participants