This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

What about using namespace !!!.!!!?

What about using namespace !!!.!!!?

I haven't checked, but I'm fairly certain it's still allowed with this change. While it would certainly be nice if the parser prevented every invalid namespace entry, the main purpose of this change is just to fix the exception thrown when specific characters are used, see: https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/parser/ast.cs#L8457

GitHub

PowerShell/ast.cs at master · PowerShell/PowerShell

PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

For this issue, there are 2 potential fixes:

• make using namespace to error on invalid namespace names
• make the constructor of TypeName to tolerate invalid namespace names
  It feels to me option 1 is the right one to go with, but it's not trivial to validate if the namespace name is legitimate using the C# identifier definition.

Although @daxian-dbw pointed out that the first solution is preferable I think full solution should include both since it is impossible to manually check whether a string valid namespace without direct test with the constructor.
I'd prefer to do formal notation check on first step (with Regex) and then try constructor. This allows us to make useful error messages and suggestions and block incorrect inputs.

@iSazonov Using statements cannot be so strict during parsing because types can be created on the fly like this:

using namespace DemoNamespace
Add-Type -TypeDefinition @'
namespace DemoNamespace
{
    public class Hello
    {
        public static void World()
        {
            System.Console.WriteLine("Hello World");
        }
    }
}
'@
[Hello]::World()

I agree that the syntax check itself could be made more thorough, but I don't fully understand the naming rules so it's hard for me to write a regex pattern that I'm confident only blocks invalid values.

@MartinGC94 I mean the checks are performed at different times.
As for Regex we could start with simple one. I don't think we should be so strong as C# compiler. Our intention is to make useful error message instead of unexpected exception.

@iSazonov If the intent is just to prevent unintentional exceptions then the current checks should be fine. Invalid namespaces like using namespace !!!.!!! won't throw any errors but they obviously won't do anything useful either.
If you want, I can easily write a more through regex pattern with the rules I think namespace identifiers have, I'm just saying there's a good chance I'll make a mistake because I don't fully understand the spec.

You could play with [regex]::IsMatch("a.q.w", "^([\p{L}_][\p{L}\p{Pc}\p{Nd}\p{Cf}\p{M}]*)(\.[\p{L}_][\p{L}\p{Pc}\p{Nd}\p{Cf}\p{M}]*)*$")

For reference https://www.regular-expressions.info/unicode.html

Regex Tutorial - Unicode Characters and Properties

This PR has 65 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Label      : Small
Size       : +47 -18
Percentile : 26%

Total files changed: 3

Change summary by file extension:
.cs : +43 -18
.ps1 : +4 -0

Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

while I agree that this is a problem, I'm concerned about this approach. It seems that the issue is that new TypeName is not expected to throw and it clearly will under some circumstances. A simpler approach might be to address that.
I tried this approach and it seems to produce the proper result:

TypeName newTypeName;
try
{
   newTypeName = new TypeName(typeName.Extent, newTypeNameToSearch);
}
catch (Exception e)
{
   exception = e;
   break;
}

this is around line 400 in engine/parser/TypeResolver.cs
here's the error message with this implementation:

PS> using namespace [system];[s                   
ParserError: 
Line |
   1 |  using namespace [system];[s
     |                             ~
     | Missing ] at end of attribute or type literal.

After more consideration, a TypeName.TryCreate might be an even better approach (to enable better inlining) then checking to see if we were able to create it.

@JamesWTruher I agree that adding TypeName.TryCreate could be useful but should it be in place of the existing changes that makes the namespace parsing more strict? Or in addition to it? I think it would be useful to have the parser show an error for invalid namespaces.

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

Superseded by #21162. @MartinGC94 and @iSazonov Thanks for your initial investigation! I have called out the thanks to both of you in the new PR.

📣 Hey @MartinGC94, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

Microsoft Forms