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

tighten up AnalyzerOptions #8605

Closed
heejaechang opened this issue Feb 11, 2016 · 6 comments
Closed

tighten up AnalyzerOptions #8605

heejaechang opened this issue Feb 11, 2016 · 6 comments

Comments

@heejaechang
Copy link
Contributor

we should do what @tmat suggested.

...

    public AnalyzerOptions AnalyzerOptions
    {
        get
        {
            if (analyzerOptions == null)
            {
                this.analyzerOptions = new AnalyzerOptions(this.additionalDocumentStates.Values.Select(d => new AdditionalDocumentStream(d)), globalOptions: null);
            }

            return analyzerOptions;
        }
    }

    private static AnalyzerOptions CreateAnalyzerOptions(ImmutableDictionary<DocumentId, TextDocumentState> additionalDocStates)
    {
        return new AnalyzerOptions(additionalDocStates.Values.Select(d => new AdditionalDocumentStream(d)), globalOptions: null);
    }

I couple of concerns

  1. CreateAnalyzerOptions seems to be unreachable and duplicating logic in the getter – I think it would be better to change analyzerOptions to readonly Lazy lazyAnalyzerOptions.
  2. The order of documents in the list passed to AnalyzerOptions is non-deterministic. Since this is then exposed as AnalyzerOptions.AdditionalFiles API it seems like we should order it in some way. Especially since AnalyzerOptions have value equality semantics.
  3. AnalyzerOptions should be sealed and implement IEquatable

Tomas

@mavasani
Copy link
Contributor

Note that 3. is not possible until we have some way for analyzers running in the IDE to get workspace options (simplify type name analyzer needs it) OR switch such analyzers to be DocumentAnalyzers that can run only in the IDE.

@heejaechang
Copy link
Contributor Author

@mavasani I think I heard that analyzer will support options (I mean like IDE workspace options). once that is supported, we might not need WorkspaceOptions anymore?

@mavasani
Copy link
Contributor

@heejaechang Yes that should fix it.

@srivatsn srivatsn added the Bug label Feb 16, 2016
@srivatsn srivatsn added this to the 1.3 milestone Feb 16, 2016
@mavasani
Copy link
Contributor

mavasani commented Apr 7, 2016

@srivatsn this item depends on the work to support analyzer options, we can then get rid of WorkspaceAnalyzerOptions and make AnalyzerOptions sealed.

Should we move this issue out of 1.3? Its unlikely we will complete the above 2 items by 1.3.

@srivatsn srivatsn modified the milestones: 2.0 (RC), 1.3 Apr 11, 2016
@mavasani
Copy link
Contributor

Moving this out of 2.0 to Unknown - not sure if we can even get to supporting analyzer options by 2.0

@mavasani
Copy link
Contributor

This is no longer relevant. WorkspaceAnalyzerOptions cannot be removed based on #64046 (comment). Hence, AnalyzerOptions cannot be sealed (it will also be a breaking change to seal a shipped public type).

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

No branches or pull requests

3 participants