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

Do not treat text as json when passed to api that explicit says it is not #74019

Merged
merged 6 commits into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Immutable;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Classification;
using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
using Microsoft.CodeAnalysis.Editor.Shared.Utilities;
using Microsoft.CodeAnalysis.Editor.Tagging;
using Microsoft.CodeAnalysis.Editor.UnitTests;
using Microsoft.CodeAnalysis.Editor.UnitTests.Classification;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Remote.Testing;
using Microsoft.CodeAnalysis.Shared.Extensions;
Expand All @@ -27,7 +29,7 @@
namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Classification;

[Trait(Traits.Feature, Traits.Features.Classification)]
public partial class SemanticClassifierTests : AbstractCSharpClassifierTests
public sealed partial class SemanticClassifierTests : AbstractCSharpClassifierTests
Copy link
Member

Choose a reason for hiding this comment

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

looks like this forces the TestEmbeddedCSharpAsync protection to go public

{
protected override async Task<ImmutableArray<ClassifiedSpan>> GetClassificationSpansAsync(string code, ImmutableArray<TextSpan> spans, ParseOptions? options, TestHost testHost)
{
Expand All @@ -37,6 +39,14 @@ protected override async Task<ImmutableArray<ClassifiedSpan>> GetClassificationS
return await GetSemanticClassificationsAsync(document, spans);
}

private new Task TestAsync(
[StringSyntax("C#-Test")] string code,
TestHost testHost,
params FormattedClassification[] expected)
{
return base.TestAsync(code, testHost, expected);
}

[Theory, CombinatorialData]
public async Task GenericClassDeclaration(TestHost testHost)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,28 @@ void Goo()
Json.Array("]"));
}

[Theory, CombinatorialData]
[WorkItem("https://github.com/dotnet/roslyn/issues/74020")]
public async Task TestJsonOnApiWithStringSyntaxAttribute_OtherLanguage_Field(TestHost testHost)
{
await TestAsync(
"""
using System.Diagnostics.CodeAnalysis;

class Program
{
[StringSyntax("notjson")]
private string field;
void Goo()
{
[|this.field = @"[{ 'goo': 0}]";|]
}
}
""" + EmbeddedLanguagesTestConstants.StringSyntaxAttributeCodeCSharp,
testHost,
Field("field"));
}

[Theory, CombinatorialData]
public async Task TestJsonOnApiWithStringSyntaxAttribute_Property(TestHost testHost)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,14 @@

namespace Microsoft.CodeAnalysis.Features.EmbeddedLanguages;

internal abstract class AbstractLanguageDetector<TOptions>
internal abstract class AbstractLanguageDetector<TOptions>(
EmbeddedLanguageInfo info,
ImmutableArray<string> languageIdentifiers,
EmbeddedLanguageCommentDetector commentDetector)
where TOptions : struct, Enum
{
protected readonly EmbeddedLanguageInfo Info;

private readonly EmbeddedLanguageDetector _detector;

protected AbstractLanguageDetector(
EmbeddedLanguageInfo info,
ImmutableArray<string> languageIdentifiers,
EmbeddedLanguageCommentDetector commentDetector)
{
Info = info;
_detector = new EmbeddedLanguageDetector(info, languageIdentifiers, commentDetector);
}
protected readonly EmbeddedLanguageInfo Info = info;
protected readonly EmbeddedLanguageDetector Detector = new EmbeddedLanguageDetector(info, languageIdentifiers, commentDetector);

/// <summary>
/// Whether or not this is an argument to a well known api for this language (like Regex.Match or JToken.Parse).
Expand Down Expand Up @@ -64,7 +57,7 @@ public bool IsEmbeddedLanguageToken(

// First check for the standard pattern of either a `// lang=...` comment or an API annotated with the
// [StringSyntax] attribute.
if (_detector.IsEmbeddedLanguageToken(token, semanticModel, cancellationToken, out _, out var stringOptions))
if (Detector.IsEmbeddedLanguageToken(token, semanticModel, cancellationToken, out _, out var stringOptions))
{
// If we got string-options back, then we were on a comment string (e.g. `// lang=regex,option1,option2`).
// Attempt to convert the string options to actual options requested.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,19 @@ public bool IsEmbeddedLanguageToken(
return LanguageIdentifiers.Contains(identifier);
}

public string? TryGetEmbeddedLanguageTokenIdentifier(
SyntaxToken token,
SemanticModel semanticModel,
CancellationToken cancellationToken)
{
return IsEmbeddedLanguageTokenWorker(token, semanticModel, cancellationToken, out var identifier, out _)
? identifier
: null;
}

public bool IsEmbeddedLanguageIdentifier(string identifier)
=> LanguageIdentifiers.Contains(identifier);

private bool IsEmbeddedLanguageTokenWorker(
SyntaxToken token,
SemanticModel semanticModel,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,14 @@ protected override JsonOptions GetStringSyntaxDefaultOptions()
return result;

if (includeProbableStrings && IsProbablyJson(token, out var tree))
return tree;
{
// We have a string that looks like json. Treat it as such *unless* we see that it was *explicitly* marked
// as belonging to some other language. This allows us to light up on strings that are very likely to be
// json while not misclassifying strings that are actually meant to be something else.
var languageIdentifier = this.Detector.TryGetEmbeddedLanguageTokenIdentifier(token, semanticModel, cancellationToken);
if (languageIdentifier is null || this.Detector.IsEmbeddedLanguageIdentifier(languageIdentifier))
return tree;
}

return null;
}
Expand Down
Loading