Skip to content

Escape special characters in section name of editor config#52515

Merged
captainsafia merged 5 commits intomainfrom
safia/fix-analyzer-config
Apr 14, 2021
Merged

Escape special characters in section name of editor config#52515
captainsafia merged 5 commits intomainfrom
safia/fix-analyzer-config

Conversation

@captainsafia
Copy link
Copy Markdown

@captainsafia captainsafia commented Apr 8, 2021 •
edited
Loading

Addresses part of #52469.

@captainsafia captainsafia requested a review from chsienki April 8, 2021 22:31
@captainsafia captainsafia requested a review from a team as a code owner April 8, 2021 22:31
@ghost ghost added the Area-Analyzers label Apr 8, 2021
@jcouv jcouv added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-Compilers and removed Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Apr 9, 2021
Youssef1313
Youssef1313 reviewed Apr 9, 2021
View reviewed changes
Comment thread src/Compilers/Core/MSBuildTask/GenerateMSBuildEditorConfig.cs Outdated
Youssef1313
Youssef1313 reviewed Apr 9, 2021
View reviewed changes
Comment thread src/Compilers/Core/MSBuildTask/GenerateMSBuildEditorConfig.cs Outdated
@Youssef1313
Copy link
Copy Markdown
Member

Youssef1313 commented Apr 9, 2021

I added the ConfigWithEscapedValues in AnalyzerConfigTests to validate that the SectionNameLexer correctly processes the escaped values but it doesn't seem like the Parse helper in the AnalyzerConfigTests actually calls this codepath? It looks like it stops processing the section name here.

I believe this code path is covered by when you call TryCreateSectionNameMatcher, which should be already covered in tests. For example this test:

roslyn/src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs

Lines 401 to 411 in cf42e6b

[Fact]
public void LiteralBraces()
{
SectionNameMatcher matcher = TryCreateSectionNameMatcher("abc\\{\\}def").Value;
Assert.Equal(@"^.*/abc\{}def$", matcher.Regex.ToString());
Assert.True(matcher.IsMatch("/abc{}def"));
Assert.True(matcher.IsMatch("/subdir/abc{}def"));
Assert.False(matcher.IsMatch("/abcdef"));
Assert.False(matcher.IsMatch("/abc}{def"));
}

Youssef1313
Youssef1313 reviewed Apr 9, 2021
View reviewed changes
Comment thread src/Compilers/Core/MSBuildTaskTests/GenerateMSBuildEditorConfigTests.cs
CyrusNajmabadi
CyrusNajmabadi reviewed Apr 9, 2021
View reviewed changes
Comment thread src/Compilers/Core/MSBuildTask/GenerateMSBuildEditorConfig.cs Outdated
CyrusNajmabadi
CyrusNajmabadi reviewed Apr 9, 2021
View reviewed changes
Comment thread src/Compilers/Core/MSBuildTask/GenerateMSBuildEditorConfig.cs Outdated
CyrusNajmabadi
CyrusNajmabadi reviewed Apr 9, 2021
View reviewed changes
Comment thread src/Compilers/Core/MSBuildTask/GenerateMSBuildEditorConfig.cs Outdated
CyrusNajmabadi
CyrusNajmabadi reviewed Apr 12, 2021
View reviewed changes
Comment thread src/Compilers/Core/MSBuildTask/GenerateMSBuildEditorConfig.cs Outdated
CyrusNajmabadi
CyrusNajmabadi reviewed Apr 12, 2021
View reviewed changes
Comment thread src/Compilers/Core/MSBuildTask/GenerateMSBuildEditorConfig.cs Outdated
CyrusNajmabadi
CyrusNajmabadi reviewed Apr 12, 2021
View reviewed changes
Comment thread src/Compilers/Core/MSBuildTask/GenerateMSBuildEditorConfig.cs Outdated
pranavkm
pranavkm reviewed Apr 13, 2021
View reviewed changes
Comment thread src/Compilers/Core/MSBuildTask/GenerateMSBuildEditorConfig.cs
{
builder.Append("\\");
}
builder.Append(c);
Copy link
Copy Markdown
Contributor

@pranavkm pranavkm Apr 12, 2021

Choose a reason for hiding this comment

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

Unrelated to the issue you're trying to solve, but does this need to escape slashes (\)?

Copy link
Copy Markdown
Author

@captainsafia captainsafia Apr 13, 2021

Choose a reason for hiding this comment

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

For this scenario, I tried to stick to values that were defined as special characters per the editor config specification (http://docs.editorconfig.org/en/master/editorconfig-format.html).

chsienki
chsienki approved these changes Apr 13, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@chsienki chsienki left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

cston
cston reviewed Apr 13, 2021
View reviewed changes
Comment thread src/Compilers/Core/MSBuildTask/GenerateMSBuildEditorConfig.cs Outdated
cston
cston reviewed Apr 13, 2021
View reviewed changes
Comment thread src/Compilers/Core/MSBuildTaskTests/GenerateMSBuildEditorConfigTests.cs
@captainsafia captainsafia merged commit 1fb7242 into main Apr 14, 2021
@ghost ghost added this to the Next milestone Apr 14, 2021
@Youssef1313 Youssef1313 mentioned this pull request Apr 14, 2021
Closed
@dibarbet dibarbet modified the milestones: Next, 16.10.P3 Apr 26, 2021
@captainsafia captainsafia mentioned this pull request Jul 1, 2021
Merged
@JoeRobich JoeRobich deleted the safia/fix-analyzer-config branch November 3, 2021 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chsienki chsienki chsienki approved these changes

+4 more reviewers

@pranavkm pranavkm pranavkm left review comments

@Youssef1313 Youssef1313 Youssef1313 left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-Analyzers Area-Compilers

Projects

None yet

Milestone

16.10.P3

Development

Successfully merging this pull request may close these issues.

8 participants

@captainsafia @Youssef1313 @pranavkm @CyrusNajmabadi @cston @chsienki @dibarbet @jcouv