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

Replace AnalyzerConfig.GetAnalyzerConfigOptions() with a new API #31578

Open
wants to merge 5 commits into
base: features/editorconfig-in-compiler
from

Conversation

Projects
None yet
3 participants
@jasonmalinowski
Member

jasonmalinowski commented Dec 6, 2018

GetAnalyzerConfigOptions() was doing two separate things:

  1. Parse and process the .editorconfig files.
  2. Compute how those files apply to source file paths.

This breaks these out into two steps: there's now a Merge method that does the first step, and you can then ask the result of that for the second step. This is intended to better fit the IDE model, where we will do step 1 and then share that to the different places that lazily do step 2. It also allows for parallelization of step 2 across files.

@jasonmalinowski jasonmalinowski requested a review from agocke Dec 6, 2018

@jasonmalinowski jasonmalinowski requested a review from dotnet/roslyn-compiler as a code owner Dec 6, 2018

@jasonmalinowski jasonmalinowski force-pushed the jasonmalinowski:introduce-editorconfig-merge-api branch from 60500d8 to ad05a09 Dec 7, 2018

@jasonmalinowski jasonmalinowski self-assigned this Dec 7, 2018

@jasonmalinowski jasonmalinowski force-pushed the jasonmalinowski:introduce-editorconfig-merge-api branch 3 times, most recently from 5d3f760 to 9515cc4 Dec 7, 2018

@jasonmalinowski jasonmalinowski force-pushed the jasonmalinowski:introduce-editorconfig-merge-api branch from 9515cc4 to 01c8b5a Dec 11, 2018

/// <summary>
/// Represents a set of <see cref="AnalyzerConfig"/> that can be applied to a given source location.
/// </summary>
public sealed class MergedAnalyzerConfig

This comment has been minimized.

@agocke

agocke Dec 12, 2018

Contributor
  1. Why not a struct?
  2. Let's call this AnalyzerConfigSet
  3. The doc comment isn't that useful. Maybe mention something about how it represents all the AnalyzerConfigs in a given directory tree?
Show resolved Hide resolved src/Compilers/Core/Portable/CommandLine/AnalyzerConfig.cs Outdated
Show resolved Hide resolved src/Compilers/Core/Portable/CommandLine/MergedAnalyzerConfig.cs Outdated
@@ -15,6 +15,8 @@ public abstract class AnalyzerConfigOptions
/// </summary>
public static StringComparer KeyComparer { get; } = AnalyzerConfig.Section.PropertiesKeyComparer;

internal static ImmutableDictionary<string, string> EmptyDictionary = ImmutableDictionary.Create<string, string>(KeyComparer);

This comment has been minimized.

@agocke

agocke Dec 12, 2018

Contributor

Let's just call this Empty

_analyzerMatchers = allMatchers.ToImmutableAndFree();
}

public AnalyzerConfigOptionsResult GetOptionsForSourcePath(string sourcePath)

This comment has been minimized.

@agocke

agocke Dec 12, 2018

Contributor

Public APIs need doc comments.

Show resolved Hide resolved src/Compilers/Core/Portable/CommandLine/AnalyzerConfigOptionsResult.cs Outdated

@agocke agocke self-assigned this Dec 12, 2018

jasonmalinowski added some commits Dec 7, 2018

Replace AnalyzerConfig.GetAnalyzerConfigOptions() with a new API
GetAnalyzerConfigOptions() was doing two separate things:

1. Parse and process the .editorconfig files.
2. Compute how those files apply to source file paths.

This breaks these out into two steps: there's now a Merge method that
does the first step, and you can then ask the result of that for the
second step. This is intended to better fit the IDE model, where we will
do step 1 and then share that to the different places that lazily do
step 2. It also allows for parallelization of step 2 across files.
Ensure AnanlyzerConfigOptionsResult always has non-null members
This simplifies any downstream consumers from having to know how to
deal with the null case. Since the dictionaries also have special
comparers, anybody who needed to deal with the null-means-empty case
would be running into a problem if they tried to craft their own
dictionary versus amending an existing one.

@jasonmalinowski jasonmalinowski force-pushed the jasonmalinowski:introduce-editorconfig-merge-api branch from 01c8b5a to b447a8d Dec 13, 2018

@agocke

agocke approved these changes Dec 13, 2018

LGTM

{
_analyzerConfigs = analyzerConfigs;

var allMatchers = ArrayBuilder<ImmutableArray<SectionNameMatcher?>>.GetInstance();

This comment has been minimized.

@agocke

agocke Dec 13, 2018

Contributor

GetInstance(_analyzerConfigs.Length)?

@agocke agocke requested a review from jaredpar Dec 13, 2018

@agocke

This comment has been minimized.

Contributor

agocke commented Dec 14, 2018

@jaredpar Can you take a look at this? I think it's holding up some other PRs.

/// <summary>
/// Represents a set of <see cref="AnalyzerConfig"/> that can be applied to a given source location.
/// </summary>
public sealed class AnalyzerConfigSet

This comment has been minimized.

@agocke

agocke Dec 16, 2018

Contributor

I still think this should be a readonly struct.

This comment has been minimized.

@jasonmalinowski

jasonmalinowski Dec 17, 2018

Member

What should the behavior be for the default struct then?

This comment has been minimized.

@jasonmalinowski

jasonmalinowski Dec 17, 2018

Member

For the method on it, and all future methods we'll later add, of course?

This comment has been minimized.

@jasonmalinowski

jasonmalinowski Dec 17, 2018

Member

My thought here is given this is a type that will get maybe one allocation per project, for the lifetime of Visual Studio, it's unclear to me why we're concerned about allocations, especially given the behavior then gets strange. If we want to add additional caching that does things lazily under the covers here, that might make us regret making this a struct because then we'll have multiple fields, and that that means for threadsafety, etc.

This comment has been minimized.

@jasonmalinowski

jasonmalinowski Dec 17, 2018

Member

Consider too the IDE will have an AsyncLazy<AnalyzerConfigSet>, and if you make it a struct it'll just have to JIT a custom copy rather than reusing the reference __Canon one, thereby increasing JIT time, start time, and probably offsetting the memory gained by this with the JITted method overhead. That type is not small.

@agocke

agocke approved these changes Dec 18, 2018

OK

agocke added some commits Dec 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment