Cache the unescaped form of globalconfig section names

[jasonmalinowski]

We were unescaping section names each time we were trying to find the applicable config rules for a file; if we were computing options for each file in a compilation this resulted in a huge amount of duplicate work.

Fixes AB#1472298

[jasonmalinowski]

@tmat @sharwell for review.

[chsienki]

Do we actually use the unescaped section name anywhere? Consider just updating the namedSections in place with the unescaped name.

[chsienki]

Hmm thinking about it, maybe we can just do the escaping as we build the global config. I don't think the unescaped name is ever used anywhere, so we can probably just do it once upfront instead.

[jasonmalinowski]

@chsienki I was somewhat worried whether that might introduce new bugs since it means some section names are escaped and some aren't and it's more or less up to you to know what's what. Now that said, there's other code here:

roslyn/src/Compilers/Core/Portable/CommandLine/AnalyzerConfigSet.cs

Lines 572 to 578 in cc888b2

	if (!_values.TryGetValue(section.Name, out var sectionDict))
	{
	sectionDict = ImmutableDictionary.CreateBuilder<string, (string, string, int)>(Section.PropertiesKeyComparer);
	_values.Add(section.Name, sectionDict);
	}
	_duplicates.TryGetValue(section.Name, out var duplicateDict);

That's using the escaped string when trying to merge sections. Looking at our lexer \ is used to escape and character, even if it otherwise doesn't require escaping, so it would mean Foo and F\oo will convert to the same string, but we won't merge the properties because we're dealing with escaped and not unescaped strings.

I can't tell if that's a bug in our lexer that we even allow such a construct (the .editorconfig specification says "Special characters can be escaped with a backslash so they won't be interpreted as wildcard patterns.", which isn't "all characters can be escaped..."), but in general this is arguing for doing the escaping earlier, I think.

[chsienki]

@jasonmalinowski Yeah, the specification is annoyingly vague on that front. I'm not sure where I land on which way we should interpret it, but think the current behavior is probably fine. If you want a backslash you can escape it, so its at least consistent and keeps the lexer simpler.

That being said global configs are already not editor configs, so I think as long as we decide on the explicit set of rules there (and document them) we're probably ok.

[jasonmalinowski]

@chsienki Yeah, I'm OK keeping the lexer behavior. Does that then mean we have a bug for the Foo and Fo\o example? Because if we're calling that a bug then that absolutely motivates me to redo this PR to do the unescape process earlier and just update the section name directly since the more look the more I'm finding problems here otherwise.

[chsienki]

Yeah, I think doing it earlier is the right approach here.

[chsienki]

FWIW, I just went to check about path separators, and in calling out that \ is not a valid path seperator it actually lists its as \\

So I think the intention behind the spec is also that a backslash char is always an escape character. And we can just process up front and be correct / compliant.

[jasonmalinowski]

Alright, time to rewrite this PR then!

[jasonmalinowski]

Ah, my bad we don't have the bug. Once we're actually looping through global configs we don't break on the first one but keep searching regardless. Still going to simplify this though that way.

Fundamentally different approach taken, so resetting approval.

[jasonmalinowski]

@sharwell @chsienki Rewritten to the other approach of just changing the section name right away.

[jasonmalinowski]

Thanks @jaredpar and @chsienki for the reviews!