Add a general-purpose property bag to Compilation.

[gafter]

Fixes #5542. This is a draft for API feedback. I need help with naming.

@dotnet/roslyn-compiler @pharring @mattwar

[pharring]

This could be lazily initialized with LazyInitializer.EnsureInitialized so you don't pay an allocation penalty unless it's used.

[gafter]

Compilations are not created often. I'd prefer to wait until it is a demonstrated issue.

[AlekseyTs]

Actually I think @pharring has a point here. I believe, IDE often mutates compilation by using With... methods, adds syntax trees, changes options, adds references, etc. That is why we are trying to reuse as much computed data as possible when we implement these methods. Allocating a new dictionary per each call like this is probably an overkill.

[pharring]

How about "Annotation" or "CompilationAnnotation"?
i.e. TryAddAnnotation / TryRemoveAnnotation / GetOrCreateAnnotation / TryGetAnnotation

[gafter]

Perhaps CompilationLocal, by analogy with ThreadLocal?

[tmeschter]

What's the rationale for putting _compilationStuff in Compilation but making the other members (TryGetStuff, TrySetStuff, etc.) extension methods?

[gafter]

I want Compilation's API to be immutable. These extension methods act as if they are implemented using an external conditional weak table, but the implementation is in fact more efficient.

[AlekseyTs]

Should new compilations derived from an existing compilation by means of With... methods inherit the properties?

[AlekseyTs]

The name of the method implies that one can use it to change the value associated with the key, but this is not what implementation does. Consider renaming to TryAddStuff or adjusting implementation accordingly.

[pharring]

👍 TryAdd....

[pharring]

@AlekseyTs No, these properties should absolutely NOT be inherited. As @gafter said elsewhere, the field on Compilation is implementation detail. The properties should have the same behavior as-if they were stored in an external ConditionalWeakTable.

[AlekseyTs]

@pharring Where the field is stored has nothing to do with my question. It would be valid even if the values were stored in an external ConditionalWeakTable. The question is whether we want to inherit the properties not the dictionary.

[pharring]

@AlekseyTs No, we do not want to inherit the properties.

[gafter]

@mattwar What would you think of CompilationProperties instead of CompilationStuff?

[tmat]

@gafter I don't see the value of pretending that the Compilation is immutable when it's in fact not.

[jaredpar]

What isn't immutable about it?

[tmat]

You put stuff in it?

[jaredpar]

Ah. I thought you were saying that the Compilation before this change wasn't immutable.

[tmat]

@jaredpar I meant with the change. I don't like adding extension methods for types that we control.

[tmat]

What about defining CompilationProperties as an instance type and adding Compilation.WithProperties(CompilationProperties) method where you pass in an instance of CompilationProperties. The instance would be mutable. The compilation itself won't.

[gafter]

@tmat The intent is that Compilation presents an immutable API, not that its implementation doesn't change any bits. These extension methods act as if they are implemented using an external conditional weak table, so that Compilation can continue to present an immutable API. Would you feel differently about it if it were implemented using a CWT?

[tmat]

@gafter The purpose of extension methods is to "add" methods to a type that is in a different library. That's not the case here.

[tmat]

@gafter Yes, it addresses my concern of using extension methods. Still bothers me that we are adding mutable state. Feels like the extra data should go to some analyzer context and other stateful objects that features work with. @pharring it'd be useful to list all the use cases for this API and see in which cases there already is or can be added a storage place that could hold on the bag.

[pharring]

@tmat I added that information to #5542 for you.

[gafter]

OK, all, I've revised the API. Please have another look.

[AlekseyTs]

Copyright?

[agocke]

If we want to keep the accessing methods taking the compilation I agree we can get rid of instantiation and simply use a static class. The design I had in mind was that the a static Get(Compilation compilation) method returned an instance of a CompilationContext this context had the accessors on it and a private WeakRef to the compilation. Whenever the user wanted to access their compilation stuff they would request a Context object, which they would then query.

Now that I've seen it, I'm not opposed to the full-static version. It loses the abstract concept of having a nameable "black box" that you can point to and say "my stuff is stored in and retrieved from there" but it saves allocations and prevents the situation where someone stashes a context somewhere and the compilation gets garbage collected and now we have to produce some kind of error about exceeding lifetimes.

[agocke]

Also, I'm still not wed to CompilationContext. It feels wrong -- "Context" often implies some kind of environment or (in PL) lexical scope. This is neither. It's more like a store or bag, but I'm not sure that CompilationStore or CompilationBag is any better.

MyCompilationCrap is growing on me.

[pharring]

My original comment is gone now, but I'll re-iterate: Make this a lazily allocated dictionary. An empty ConcurrentDictionary carries quite a bit of overhead.

The canonical pattern is:

private ConcurrentDictionary<object, object> _compilationContext;
internal ConcurrentDictionary<object, object> CompilationContext => LazyInitializer.EnsureInitialized(ref _compilationContext);

Another way to mitigate the allocation hit for an empty ConcurrentDictionary is to specify a smaller concurrency level (the default is 4 times the processor count) and initial capacity (default is 31).

[pharring]

👍 on making it a static class.

[tmat]

I'm not convinced that all the uses of CWT Paul pointed out are actually justified. Maybe they are, I don't claim to fully understand the code. But I'm wondering...

For example,
1)
http://source.roslyn.io/#Microsoft.CodeAnalysis/DiagnosticAnalyzer/AnalyzerManager.cs,67:

 var compilationActionsMap = _compilationScopeMap.GetOrAdd(analyzerAndOptions, new ConditionalWeakTable<Compilation, Task<HostCompilationStartAnalysisScope>>());
            return compilationActionsMap.GetValue(analyzerExecutor.Compilation, callback);

Couldn't the task be stored in AnalyzerExecutor? The executor holds on the compilation, why couldn't it also hold on the additional data needed for the compilation? Why is it loading it from a CWT instead?

2. http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/FindSymbols/FindReferences/DependentTypeFinder.cs,346fe98846f4a353

Has 5 CWTs, each adding some data to a compilation. Couldn't we have just one that holds on all state this component needs to add?

Also couldn't the SymbolFinder maintain a context with the same lifetime as the compilation or a project? The code seems to get a Compilation from a Project and then look up in a cache implemented as CWT. Perhaps the cache can be stored in ProjectState?

[mattwar]

How internal code keeps associated data with compilation is not interesting. We can cook up anything we want. Exposing it as a public API makes me cringe. Solve this problem in a different way. If analyzer developers need a way to keep state alive, add that to the analyzer context, but don't pollute compilation with this.

[pharring]

@JohnHamby for the analyzer side of things. An analyzer developer wants to be able to set some state in CompilationStart, modify in a Syntax or Symbol analyzer callbacks, and then report on the state in CompilationEnd. Since analyses can run concurrently over multiple compilations, people have been using the Compilation as the key into their analyzer state dictionary (in a CWT so that they don't keep Compilations alive). The problem with adding to analyzer context is you get one kind of context object for a CompilationStart/End analyzer and another for a Syntax or Symbol analyzer. We could add a property bag field to CompilationStartAnalysisContext and then thread that through to the other context objects. Presumably it's a breaking change for analyzer writers. I note that some of the context callback objects are structs which might make things more difficult.

[msJohnHamby]

The intended model for managing such state in analyzers is to create the state in a compilation start action and then access it directly in actions registered by the compilation start action. One way to do this is to put the state into a variable local to the compilation start action and have the other actions be lambdas that refer to the local.

But we all know that, so presumably there's a deeper issue for analyzer state that I'm not understanding. Sorry to be dense, but could someone explain it further?

[pharring]

There was an example on the internal discussion alias (rosdisc) recently (9/29/2015 "Scope within an run of analyzers"). In that discussion, the OP had several analyzers all using the same additional file. It was expensive to parse that file, so we were looking for a way to parse just once (per compilation) and share the result among all the analyzers. A couple of solutions were presented, one of which was to use a ConditionalWeakTable<Compilation, Options>.

[msJohnHamby]

Ah, I see--the issue is sharing state across analyzers. That is explicitly not supported, specifically because it breaks the concurrency model and generally because it breaks the model of being able to think about analyzers as independent entities.

The recommended approach to this sort of sharing is to have one analyzer with lots of actions.

[mattwar]

@pharring the context objects are designed to allow us to add properties without breaking user code.

[gafter]

I sense the prevailing wind is "Won't Fix". Is that right?

[mattwar]

That would be my choice. Which is equivalent to "Solve the problem in a different way."