vscode-powershell: Semantic Syntax is incorrectly highlighting arguments as commands

So I think we need to take this in two stages.

1. The first stage is parity: being as close to PSReadLine as possible. This will allow both the console and editing experience to be on the same playing field. (this was actually the original goal of doing this work from the beginning)

2. The second stage is improving: Once both experiences are roughly aligned, then we can go back and make the changes in both to support the language better.

@rjmholt I think what they’re getting at is that the current regex based highlighter treats the name in a function definition like a command (which doesn’t have the flag).

Personally I’d rather have the name in a function definition be parsed as a bare word than the current situation (that’s how both the ISE and PSRL work currently). But a fix for both might be just keeping track of if the previous token kind was Function. Then also use TokenFlags.CommandName.

I don’t think there’s that much backward compatibility to worry about. It’s already drastically changing most of the highlighting… and not just breaking changes like using statements with paths, escaped characters, #requires, etc.

And PSReadline is wrong about that too. Both your here and 'there' are strings. Why would you want to make them seem different? No offense intended. I mean, I did most of the regex parser, I’m not trying to blame anyone. I’m just pointing out that it’s literally a string (and now, we know that) but it’s not highlighted as one just because … we never bothered to get it right.

Yeah it’s not about how PowerShell uses it, it’s about how it’s highlighted. Categorising it as a string for highlighting purposes will make 'input' and input the same colour, whereas in PSReadLine for example, they are not at all (here vs 'there'):

Are you really arguing that since everyone else is wrong, this should be wrong too? It’s a string.

Please keep your tone civil. We can be more constructive without rhetoric like this.

Are you really arguing that since everyone else is wrong, this should be wrong too?

Nah, I’m saying people will be annoyed if it’s highlighted like every other string literal, because that’s what they’re used to.

It’s a string. The fact that PowerShell allows you to leave off the quotes doesn’t change the fact that it’s a string.

Yeah, no one here thinks it isn’t a string. Doesn’t change the fact that folks expect it to be highlighted differently 🤷

I think what they’re getting at is that the current regex based highlighter treats the name in a function definition like a command (which doesn’t have the flag).

Yes, I noticed that. But in @Jaykul’s original issue description he seems to be referring to the CommandAst situation rather than the FunctionDefinitionAst situation.

The FunctionDefinitionAst scenario is easy to address: we look back a token and see if it’s the function keyword. Given that the grammar defines the full nature of the context, rather than just the tokens, we can’t do a simple token search for all tokens, but I believe we can in this case.

A more sophisticated approach is to track our position in both the AST and the token array simulataneously and for each token look into where we are in the AST to determine the token semantics.

I’ve written an example where we track through tokens as we move through an AST here.

Or the converse, where we find the AST element corresponding to our token:

    /// <summary>
    /// An AST visitor to find the smallest AST containing a given IScriptPosition.
    /// </summary>
    internal class FindAstFromPositionVisitor : AstVisitor2
    {
        private readonly IScriptPosition _position;


        private Ast _astAtPosition;


        /// <summary>
        /// Create a FindAstFromPositionVisitor around a given position,
        /// ready to find the smallest AST containing that position.
        /// </summary>
        /// <param name="position">The position to find the containing AST of.</param>
        public FindAstFromPositionVisitor(IScriptPosition position)
        {
            _position = position;
        }


        /// <summary>
        /// Retrieve the computed smallest containing AST after a visit has been performed.
        /// </summary>
        /// <returns>The smallest AST containing the originally given position in the visited AST.</returns>
        public Ast GetAst()
        {
            return _astAtPosition;
        }


        public override AstVisitAction VisitArrayExpression(ArrayExpressionAst arrayExpressionAst) => VisitAst(arrayExpressionAst);


        public override AstVisitAction VisitArrayLiteral(ArrayLiteralAst arrayLiteralAst) => VisitAst(arrayLiteralAst);


        public override AstVisitAction VisitAssignmentStatement(AssignmentStatementAst assignmentStatementAst) => VisitAst(assignmentStatementAst);


        public override AstVisitAction VisitAttribute(AttributeAst attributeAst) => VisitAst(attributeAst);


        public override AstVisitAction VisitAttributedExpression(AttributedExpressionAst attributedExpressionAst) => VisitAst(attributedExpressionAst);


        public override AstVisitAction VisitBaseCtorInvokeMemberExpression(BaseCtorInvokeMemberExpressionAst baseCtorInvokeMemberExpressionAst) => AstVisitAction.SkipChildren;


        public override AstVisitAction VisitBinaryExpression(BinaryExpressionAst binaryExpressionAst) => VisitAst(binaryExpressionAst);


        public override AstVisitAction VisitBlockStatement(BlockStatementAst blockStatementAst) => VisitAst(blockStatementAst);


        public override AstVisitAction VisitBreakStatement(BreakStatementAst breakStatementAst) => VisitAst(breakStatementAst);


        public override AstVisitAction VisitCatchClause(CatchClauseAst catchClauseAst) => VisitAst(catchClauseAst);


        public override AstVisitAction VisitCommand(CommandAst commandAst) => VisitAst(commandAst);


        public override AstVisitAction VisitCommandExpression(CommandExpressionAst commandExpressionAst) => VisitAst(commandExpressionAst);


        public override AstVisitAction VisitCommandParameter(CommandParameterAst commandParameterAst) => VisitAst(commandParameterAst);


        public override AstVisitAction VisitConfigurationDefinition(ConfigurationDefinitionAst configurationDefinitionAst) => VisitAst(configurationDefinitionAst);


        public override AstVisitAction VisitConstantExpression(ConstantExpressionAst constantExpressionAst) => VisitAst(constantExpressionAst);


        public override AstVisitAction VisitContinueStatement(ContinueStatementAst continueStatementAst) => VisitAst(continueStatementAst);


        public override AstVisitAction VisitConvertExpression(ConvertExpressionAst convertExpressionAst) => VisitAst(convertExpressionAst);


        public override AstVisitAction VisitDataStatement(DataStatementAst dataStatementAst) => VisitAst(dataStatementAst);


        public override AstVisitAction VisitDoUntilStatement(DoUntilStatementAst doUntilStatementAst) => VisitAst(doUntilStatementAst);


        public override AstVisitAction VisitDoWhileStatement(DoWhileStatementAst doWhileStatementAst) => VisitAst(doWhileStatementAst);


        public override AstVisitAction VisitDynamicKeywordStatement(DynamicKeywordStatementAst dynamicKeywordStatementAst) => VisitAst(dynamicKeywordStatementAst);


        public override AstVisitAction VisitErrorExpression(ErrorExpressionAst errorExpressionAst) => VisitAst(errorExpressionAst);


        public override AstVisitAction VisitErrorStatement(ErrorStatementAst errorStatementAst) => VisitAst(errorStatementAst);


        public override AstVisitAction VisitExitStatement(ExitStatementAst exitStatementAst) => VisitAst(exitStatementAst);


        public override AstVisitAction VisitExpandableStringExpression(ExpandableStringExpressionAst expandableStringExpressionAst) => VisitAst(expandableStringExpressionAst);


        public override AstVisitAction VisitFileRedirection(FileRedirectionAst redirectionAst) => VisitAst(redirectionAst);


        public override AstVisitAction VisitForEachStatement(ForEachStatementAst forEachStatementAst) => VisitAst(forEachStatementAst);


        public override AstVisitAction VisitForStatement(ForStatementAst forStatementAst) => VisitAst(forStatementAst);


        public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst functionDefinitionAst) => VisitAst(functionDefinitionAst);


        public override AstVisitAction VisitFunctionMember(FunctionMemberAst functionMemberAst) => VisitAst(functionMemberAst);


        public override AstVisitAction VisitHashtable(HashtableAst hashtableAst) => VisitAst(hashtableAst);


        public override AstVisitAction VisitIfStatement(IfStatementAst ifStmtAst) => VisitAst(ifStmtAst);
    
        public override AstVisitAction VisitIndexExpression(IndexExpressionAst indexExpressionAst) => VisitAst(indexExpressionAst);


        public override AstVisitAction VisitInvokeMemberExpression(InvokeMemberExpressionAst methodCallAst) => VisitAst(methodCallAst);


        public override AstVisitAction VisitMemberExpression(MemberExpressionAst memberExpressionAst) => VisitAst(memberExpressionAst);


        public override AstVisitAction VisitMergingRedirection(MergingRedirectionAst redirectionAst) => VisitAst(redirectionAst);


        public override AstVisitAction VisitNamedAttributeArgument(NamedAttributeArgumentAst namedAttributeArgumentAst) => VisitAst(namedAttributeArgumentAst);


        public override AstVisitAction VisitNamedBlock(NamedBlockAst namedBlockAst) => VisitAst(namedBlockAst);


        public override AstVisitAction VisitParamBlock(ParamBlockAst paramBlockAst) => VisitAst(paramBlockAst);


        public override AstVisitAction VisitParameter(ParameterAst parameterAst) => VisitAst(parameterAst);


        public override AstVisitAction VisitParenExpression(ParenExpressionAst parenExpressionAst) => VisitAst(parenExpressionAst);


        public override AstVisitAction VisitPipeline(PipelineAst pipelineAst) => VisitAst(pipelineAst);


        public override AstVisitAction VisitPropertyMember(PropertyMemberAst propertyMemberAst) => VisitAst(propertyMemberAst);


        public override AstVisitAction VisitReturnStatement(ReturnStatementAst returnStatementAst) => VisitAst(returnStatementAst);


        public override AstVisitAction VisitScriptBlock(ScriptBlockAst scriptBlockAst) => VisitAst(scriptBlockAst);


        public override AstVisitAction VisitScriptBlockExpression(ScriptBlockExpressionAst scriptBlockExpressionAst) => VisitAst(scriptBlockExpressionAst);


        public override AstVisitAction VisitStatementBlock(StatementBlockAst statementBlockAst) => VisitAst(statementBlockAst);


        public override AstVisitAction VisitStringConstantExpression(StringConstantExpressionAst stringConstantExpressionAst) => VisitAst(stringConstantExpressionAst);


        public override AstVisitAction VisitSubExpression(SubExpressionAst subExpressionAst) => VisitAst(subExpressionAst);


        public override AstVisitAction VisitSwitchStatement(SwitchStatementAst switchStatementAst) => VisitAst(switchStatementAst);


        public override AstVisitAction VisitThrowStatement(ThrowStatementAst throwStatementAst) => VisitAst(throwStatementAst);


        public override AstVisitAction VisitTrap(TrapStatementAst trapStatementAst) => VisitAst(trapStatementAst);


        public override AstVisitAction VisitTryStatement(TryStatementAst tryStatementAst) => VisitAst(tryStatementAst);


        public override AstVisitAction VisitTypeConstraint(TypeConstraintAst typeConstraintAst) => VisitAst(typeConstraintAst);


        public override AstVisitAction VisitTypeDefinition(TypeDefinitionAst typeDefinitionAst) => VisitAst(typeDefinitionAst);


        public override AstVisitAction VisitTypeExpression(TypeExpressionAst typeExpressionAst) => VisitAst(typeExpressionAst);


        public override AstVisitAction VisitUnaryExpression(UnaryExpressionAst unaryExpressionAst) => VisitAst(unaryExpressionAst);


        public override AstVisitAction VisitUsingExpression(UsingExpressionAst usingExpressionAst) => VisitAst(usingExpressionAst);


        public override AstVisitAction VisitUsingStatement(UsingStatementAst usingStatementAst) => VisitAst(usingStatementAst);


        public override AstVisitAction VisitVariableExpression(VariableExpressionAst variableExpressionAst) => VisitAst(variableExpressionAst);


        public override AstVisitAction VisitWhileStatement(WhileStatementAst whileStatementAst) => VisitAst(whileStatementAst);


        private AstVisitAction VisitAst(Ast ast)
        {
            if (!AstContainsPosition(ast))
            {
                return AstVisitAction.SkipChildren;
            }


            _astAtPosition = ast;
            return AstVisitAction.Continue;
        }


        private bool AstContainsPosition(Ast ast)
        {
            return IsBefore(ast.Extent.StartScriptPosition, _position)
                && IsBefore(_position, ast.Extent.EndScriptPosition);
        }


        private static bool IsBefore(IScriptPosition left, IScriptPosition right)
        {
            return left.LineNumber < right.LineNumber
                || (left.LineNumber == right.LineNumber && left.ColumnNumber <= right.ColumnNumber);
        }
    }