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

Don't run code in the async/await highlighter unless on one of those keywords. #73721

Merged
merged 11 commits into from
May 26, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable disable

using System;
using System.Collections.Generic;
using System.Composition;
Expand All @@ -14,22 +12,22 @@
using Microsoft.CodeAnalysis.Highlighting;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Collections;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis.CSharp.KeywordHighlighting.KeywordHighlighters;

[ExportHighlighter(LanguageNames.CSharp), Shared]
internal class AsyncAwaitHighlighter : AbstractKeywordHighlighter
[method: ImportingConstructor]
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
internal class AsyncAwaitHighlighter() : AbstractKeywordHighlighter(findInsideTrivia: false)
{
private static readonly ObjectPool<Stack<SyntaxNode>> s_stackPool
= SharedPools.Default<Stack<SyntaxNode>>();

[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
public AsyncAwaitHighlighter()
{
}
protected override bool ContainsHighlightableToken(ref TemporaryArray<SyntaxToken> tokens)
=> tokens.Any(static t => t.Kind() is SyntaxKind.AwaitKeyword or SyntaxKind.AsyncKeyword);

protected override bool IsHighlightableNode(SyntaxNode node)
=> node.IsReturnableConstructOrTopLevelCompilationUnit();
Expand All @@ -45,9 +43,8 @@ protected override void AddHighlightsForNode(SyntaxNode node, List<TextSpan> hig

private static IEnumerable<SyntaxNode> WalkChildren(SyntaxNode node)
{
using var pooledObject = s_stackPool.GetPooledObject();
using var _ = s_stackPool.GetPooledObject(out var stack);

var stack = pooledObject.Object;
stack.Push(node);

while (stack.TryPop(out var current))
Expand All @@ -58,16 +55,12 @@ private static IEnumerable<SyntaxNode> WalkChildren(SyntaxNode node)
// order, which is nicer when debugging and understanding the results produced.
foreach (var child in current.ChildNodesAndTokens().Reverse())
{
if (child.IsNode)
if (child.AsNode(out var childNode))
{
var childNode = child.AsNode();

// Only process children if they're not the start of another construct
// that async/await would be related to.
if (!childNode.IsReturnableConstruct())
{
stack.Push(childNode);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,10 @@
namespace Microsoft.CodeAnalysis.CSharp.KeywordHighlighting.KeywordHighlighters;

[ExportHighlighter(LanguageNames.CSharp), Shared]
internal class ConditionalPreprocessorHighlighter : AbstractKeywordHighlighter<DirectiveTriviaSyntax>
[method: ImportingConstructor]
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
internal sealed class ConditionalPreprocessorHighlighter() : AbstractKeywordHighlighter<DirectiveTriviaSyntax>
{
[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
public ConditionalPreprocessorHighlighter()
{
}

protected override void AddHighlights(
DirectiveTriviaSyntax directive, List<TextSpan> highlights, CancellationToken cancellationToken)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,18 @@
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Highlighting;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Shared.Collections;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis.CSharp.KeywordHighlighting;

[ExportHighlighter(LanguageNames.CSharp), Shared]
internal class IfStatementHighlighter : AbstractKeywordHighlighter<IfStatementSyntax>
[method: ImportingConstructor]
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
internal sealed class IfStatementHighlighter() : AbstractKeywordHighlighter<IfStatementSyntax>(findInsideTrivia: false)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i picked the highlighters to update based on if i saw the in the traces.

{
[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
public IfStatementHighlighter()
{
}
protected override bool ContainsHighlightableToken(ref TemporaryArray<SyntaxToken> tokens)
=> tokens.Any(static t => t.Kind() is SyntaxKind.IfKeyword or SyntaxKind.ElseKeyword);

protected override void AddHighlights(
IfStatementSyntax ifStatement, List<TextSpan> highlights, CancellationToken cancellationToken)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable disable

using System;
using System.Collections.Generic;
using System.Composition;
Expand All @@ -13,18 +11,25 @@
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Highlighting;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Shared.Collections;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis.CSharp.KeywordHighlighting.KeywordHighlighters;

[ExportHighlighter(LanguageNames.CSharp), Shared]
internal class LoopHighlighter : AbstractKeywordHighlighter
[method: ImportingConstructor]
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
internal sealed class LoopHighlighter() : AbstractKeywordHighlighter(findInsideTrivia: false)
{
[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
public LoopHighlighter()
{
}
protected override bool ContainsHighlightableToken(ref TemporaryArray<SyntaxToken> tokens)
=> tokens.Any(static t => t.Kind()
is SyntaxKind.DoKeyword
or SyntaxKind.ForKeyword
or SyntaxKind.ForEachKeyword
or SyntaxKind.WhileKeyword
or SyntaxKind.BreakKeyword
or SyntaxKind.ContinueKeyword
or SyntaxKind.SemicolonToken);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SyntaxKind.SemicolonToken

Can you clarify what this means in practice? If a semicolon is at the caret position, this highlighter will always be asked?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. that's correct. that's because the loop highlighter will highlight if you're next to things like break; that's ok. this is still a tiny subset of all tokens.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the main goal here is to stop runnin all these highlighters unnecessarily on tokens they will never report results for. that's just pointless.


protected override bool IsHighlightableNode(SyntaxNode node)
=> node.IsContinuableConstruct();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable disable

using System;
using System.Collections.Generic;
using System.Composition;
Expand All @@ -13,18 +11,24 @@
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Highlighting;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Shared.Collections;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis.CSharp.KeywordHighlighting.KeywordHighlighters;

[ExportHighlighter(LanguageNames.CSharp), Shared]
internal class SwitchStatementHighlighter : AbstractKeywordHighlighter<SwitchStatementSyntax>
[method: ImportingConstructor]
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
internal sealed class SwitchStatementHighlighter() : AbstractKeywordHighlighter<SwitchStatementSyntax>(findInsideTrivia: false)
{
[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
public SwitchStatementHighlighter()
{
}
protected override bool ContainsHighlightableToken(ref TemporaryArray<SyntaxToken> tokens)
=> tokens.Any(static t => t.Kind()
is SyntaxKind.SwitchKeyword
or SyntaxKind.CaseKeyword
or SyntaxKind.DefaultKeyword
or SyntaxKind.SemicolonToken
or SyntaxKind.BreakKeyword
or SyntaxKind.GotoKeyword);

protected override void AddHighlights(
SwitchStatementSyntax switchStatement, List<TextSpan> spans, CancellationToken cancellationToken)
Expand Down Expand Up @@ -63,7 +67,7 @@
// but if the label is missing, we do highlight 'goto' assuming it's more likely that
// the user is in the middle of typing 'goto case' or 'goto default'.
if (gotoStatement.Kind() is SyntaxKind.GotoCaseStatement or SyntaxKind.GotoDefaultStatement ||
gotoStatement.Expression.IsMissing)

Check failure on line 70 in src/Features/CSharp/Portable/Highlighting/KeywordHighlighters/SwitchStatementHighlighter.cs

View check run for this annotation

Azure Pipelines / roslyn-CI (Correctness Correctness_Analyzers)

src/Features/CSharp/Portable/Highlighting/KeywordHighlighters/SwitchStatementHighlighter.cs#L70

src/Features/CSharp/Portable/Highlighting/KeywordHighlighters/SwitchStatementHighlighter.cs(70,17): error CS8602: (NETCORE_ENGINEERING_TELEMETRY=Build) Dereference of a possibly null reference.

Check failure on line 70 in src/Features/CSharp/Portable/Highlighting/KeywordHighlighters/SwitchStatementHighlighter.cs

View check run for this annotation

Azure Pipelines / roslyn-CI (Correctness Correctness_Analyzers)

src/Features/CSharp/Portable/Highlighting/KeywordHighlighters/SwitchStatementHighlighter.cs#L70

src/Features/CSharp/Portable/Highlighting/KeywordHighlighters/SwitchStatementHighlighter.cs(70,17): error CS8602: (NETCORE_ENGINEERING_TELEMETRY=Build) Dereference of a possibly null reference.
{
var start = gotoStatement.GotoKeyword.SpanStart;
var end = !gotoStatement.CaseOrDefaultKeyword.IsKind(SyntaxKind.None)
Expand All @@ -82,13 +86,13 @@
continue;

var child = childNodeOrToken.AsNode();
var highlightBreaksForChild = highlightBreaks && !child.IsBreakableConstruct();

Check failure on line 89 in src/Features/CSharp/Portable/Highlighting/KeywordHighlighters/SwitchStatementHighlighter.cs

View check run for this annotation

Azure Pipelines / roslyn-CI (Correctness Correctness_Analyzers)

src/Features/CSharp/Portable/Highlighting/KeywordHighlighters/SwitchStatementHighlighter.cs#L89

src/Features/CSharp/Portable/Highlighting/KeywordHighlighters/SwitchStatementHighlighter.cs(89,67): error CS8604: (NETCORE_ENGINEERING_TELEMETRY=Build) Possible null reference argument for parameter 'node' in 'bool SyntaxNodeExtensions.IsBreakableConstruct(SyntaxNode node)'.

Check failure on line 89 in src/Features/CSharp/Portable/Highlighting/KeywordHighlighters/SwitchStatementHighlighter.cs

View check run for this annotation

Azure Pipelines / roslyn-CI (Correctness Correctness_Analyzers)

src/Features/CSharp/Portable/Highlighting/KeywordHighlighters/SwitchStatementHighlighter.cs#L89

src/Features/CSharp/Portable/Highlighting/KeywordHighlighters/SwitchStatementHighlighter.cs(89,67): error CS8604: (NETCORE_ENGINEERING_TELEMETRY=Build) Possible null reference argument for parameter 'node' in 'bool SyntaxNodeExtensions.IsBreakableConstruct(SyntaxNode node)'.
var highlightGotosForChild = highlightGotos && !child.IsKind(SyntaxKind.SwitchStatement);

// Only recurse if we have anything to do
if (highlightBreaksForChild || highlightGotosForChild)
{
HighlightRelatedKeywords(child, spans, highlightBreaksForChild, highlightGotosForChild);

Check failure on line 95 in src/Features/CSharp/Portable/Highlighting/KeywordHighlighters/SwitchStatementHighlighter.cs

View check run for this annotation

Azure Pipelines / roslyn-CI (Correctness Correctness_Analyzers)

src/Features/CSharp/Portable/Highlighting/KeywordHighlighters/SwitchStatementHighlighter.cs#L95

src/Features/CSharp/Portable/Highlighting/KeywordHighlighters/SwitchStatementHighlighter.cs(95,46): error CS8604: (NETCORE_ENGINEERING_TELEMETRY=Build) Possible null reference argument for parameter 'node' in 'void SwitchStatementHighlighter.HighlightRelatedKeywords(SyntaxNode node, List<TextSpan> spans, bool highlightBreaks, bool highlightGotos)'.
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,54 +2,61 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable disable

using System.Collections.Generic;
using System.Threading;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Collections;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis.Highlighting;

internal abstract class AbstractKeywordHighlighter<TNode> : AbstractKeywordHighlighter where TNode : SyntaxNode
internal abstract class AbstractKeywordHighlighter<TNode>(bool findInsideTrivia = true)
: AbstractKeywordHighlighter(findInsideTrivia)
where TNode : SyntaxNode
{
protected sealed override bool IsHighlightableNode(SyntaxNode node) => node is TNode;
protected sealed override bool IsHighlightableNode(SyntaxNode node)
=> node is TNode;

protected sealed override void AddHighlightsForNode(SyntaxNode node, List<TextSpan> highlights, CancellationToken cancellationToken)
=> AddHighlights((TNode)node, highlights, cancellationToken);

protected abstract void AddHighlights(TNode node, List<TextSpan> highlights, CancellationToken cancellationToken);
}

internal abstract class AbstractKeywordHighlighter : IHighlighter
internal abstract class AbstractKeywordHighlighter(bool findInsideTrivia = true) : IHighlighter
{
private readonly bool _findInsideTrivia = findInsideTrivia;

private static readonly ObjectPool<List<TextSpan>> s_textSpanListPool = new(() => []);
private static readonly ObjectPool<List<SyntaxToken>> s_tokenListPool = new(() => []);

protected abstract bool IsHighlightableNode(SyntaxNode node);

protected virtual bool ContainsHighlightableToken(ref TemporaryArray<SyntaxToken> tokens)
=> true;

public void AddHighlights(
SyntaxNode root, int position, List<TextSpan> highlights, CancellationToken cancellationToken)
{
using (s_textSpanListPool.GetPooledObject(out var tempHighlights))
using (s_tokenListPool.GetPooledObject(out var touchingTokens))
{
AddTouchingTokens(root, position, touchingTokens);
// We only look at a max of 4 tokens (two trivia, and two non-trivia), so a temp-array is ideal here
using var touchingTokens = TemporaryArray<SyntaxToken>.Empty;
AddTouchingTokens(root, position, ref touchingTokens.AsRef());
if (!ContainsHighlightableToken(ref touchingTokens.AsRef()))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the async/await highlighter si not well written. it does all sorts of unnecessary work even when not on an appropraite keyword. this allows us to early bail out here and not do any of that work.

return;

foreach (var token in touchingTokens)
using var _2 = s_textSpanListPool.GetPooledObject(out var highlightsBuffer);
foreach (var token in touchingTokens)
{
for (var parent = token.Parent; parent != null; parent = parent.Parent)
{
for (var parent = token.Parent; parent != null; parent = parent.Parent)
if (IsHighlightableNode(parent))
{
if (IsHighlightableNode(parent))
highlightsBuffer.Clear();
AddHighlightsForNode(parent, highlightsBuffer, cancellationToken);

if (AnyIntersects(position, highlightsBuffer))
{
tempHighlights.Clear();
AddHighlightsForNode(parent, tempHighlights, cancellationToken);

if (AnyIntersects(position, tempHighlights))
{
highlights.AddRange(tempHighlights);
return;
}
highlights.AddRange(highlightsBuffer);
return;
}
}
}
Expand All @@ -61,9 +68,7 @@ private static bool AnyIntersects(int position, List<TextSpan> highlights)
foreach (var highlight in highlights)
{
if (highlight.IntersectsWith(position))
{
return true;
}
}

return false;
Expand All @@ -74,13 +79,15 @@ private static bool AnyIntersects(int position, List<TextSpan> highlights)
protected static TextSpan EmptySpan(int position)
=> new(position, 0);

internal static void AddTouchingTokens(SyntaxNode root, int position, List<SyntaxToken> tokens)
internal void AddTouchingTokens(SyntaxNode root, int position, ref TemporaryArray<SyntaxToken> tokens)
{
AddTouchingTokens(root, position, tokens, findInsideTrivia: true);
AddTouchingTokens(root, position, tokens, findInsideTrivia: false);
AddTouchingTokens(root, position, ref tokens, findInsideTrivia: true);
if (_findInsideTrivia)
AddTouchingTokens(root, position, ref tokens, findInsideTrivia: false);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no point going into trivia for most highlighters. can update others as appropraite.

}

private static void AddTouchingTokens(SyntaxNode root, int position, List<SyntaxToken> tokens, bool findInsideTrivia)
private static void AddTouchingTokens(
SyntaxNode root, int position, ref TemporaryArray<SyntaxToken> tokens, bool findInsideTrivia)
{
var token = root.FindToken(position, findInsideTrivia);
if (!tokens.Contains(token))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,8 @@ public static TextChangeRange ComputeSyntacticChangeRange(SyntaxNode oldRoot, Sy
if (oldRoot.IsIncrementallyIdenticalTo(newRoot))
return null;

using var leftOldStack = s_enumeratorPool.GetPooledObject();
using var leftNewStack = s_enumeratorPool.GetPooledObject();

var oldStack = leftOldStack.Object;
var newStack = leftNewStack.Object;
using var _1 = s_enumeratorPool.GetPooledObject(out var oldStack);
using var _2 = s_enumeratorPool.GetPooledObject(out var newStack);

oldStack.Push(oldRoot.ChildNodesAndTokens().GetEnumerator());
newStack.Push(newRoot.ChildNodesAndTokens().GetEnumerator());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,23 +37,36 @@ private static DirectiveInfo<DirectiveTriviaSyntax> GetDirectiveInfoForRoot(Synt

internal static DirectiveTriviaSyntax? GetMatchingDirective(this DirectiveTriviaSyntax directive, CancellationToken cancellationToken)
{
if (directive == null)
throw new ArgumentNullException(nameof(directive));
if (IsConditionalDirective(directive) ||
IsRegionDirective(directive))
{
var directiveSyntaxMap = GetDirectiveInfo(directive, cancellationToken).DirectiveMap;
if (directiveSyntaxMap.TryGetValue(directive, out var result))
return result;
}

var directiveSyntaxMap = GetDirectiveInfo(directive, cancellationToken).DirectiveMap;
return directiveSyntaxMap.TryGetValue(directive, out var result)
? result
: null;
return null;
}

public static ImmutableArray<DirectiveTriviaSyntax> GetMatchingConditionalDirectives(this DirectiveTriviaSyntax directive, CancellationToken cancellationToken)
{
if (directive == null)
throw new ArgumentNullException(nameof(directive));
if (IsConditionalDirective(directive))
{
var directiveConditionalMap = GetDirectiveInfo(directive, cancellationToken).ConditionalMap;
if (directiveConditionalMap.TryGetValue(directive, out var result))
return result;
}

var directiveConditionalMap = GetDirectiveInfo(directive, cancellationToken).ConditionalMap;
return directiveConditionalMap.TryGetValue(directive, out var result)
? result
: [];
return [];
}

private static bool IsRegionDirective(DirectiveTriviaSyntax directive)
=> directive?.Kind() is SyntaxKind.RegionDirectiveTrivia or SyntaxKind.EndRegionDirectiveTrivia;

private static bool IsConditionalDirective(DirectiveTriviaSyntax directive)
=> directive?.Kind()
is SyntaxKind.IfDirectiveTrivia
or SyntaxKind.ElifDirectiveTrivia
or SyntaxKind.ElseDirectiveTrivia
or SyntaxKind.EndIfDirectiveTrivia;
}
Original file line number Diff line number Diff line change
Expand Up @@ -338,9 +338,7 @@ private static bool RemovalMayIntroduceInterpolationAmbiguity(ParenthesizedExpre
// they include any : or :: tokens. If they do, we can't remove the parentheses because
// the parser would assume that the first : would begin the format clause of the interpolation.

using var pooledStack = s_nodeStackPool.GetPooledObject();
var stack = pooledStack.Object;

using var _ = s_nodeStackPool.GetPooledObject(out var stack);
stack.Push(node.Expression);

while (stack.TryPop(out var expression))
Expand Down
Loading
Loading