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

support semantic errors for script files in misc projects. #31134

Merged
merged 5 commits into from Dec 6, 2018

Conversation

@heejaechang
Contributor

heejaechang commented Nov 13, 2018

previously, we only supported syntax errors in misc project. we still want to do so for regular files, but for script files (especially for F#), we do want to support semantic errors since script file can have all required dependencies through reference directive.

this change doesn't make C# script file to suddenly show up in misc and start working. C# script file still doesn't participate in misc project. if we want to, we need to make it to work.

but for F#, this should let them to open F# script file in misc and semantic errors to work for them.

most of changes are making things static so that misc project can call existing diagnostic analyzer engine to get diagnostics.

@heejaechang heejaechang requested a review from dotnet/roslyn-ide as a code owner Nov 13, 2018

@heejaechang

This comment has been minimized.

Contributor

heejaechang commented Nov 13, 2018

this works on top of master for now. if it does solve your issue, let me see whether we can make it preview 2.

@TIHan

This comment has been minimized.

Contributor

TIHan commented Nov 13, 2018

Yes, I will look into it. Also, here is my PR where my changes for single files will go: Microsoft/visualfsharp#5845

@heejaechang

This comment has been minimized.

Contributor

heejaechang commented Nov 13, 2018

@TIHan there was a bug which I just fixed. can you try with new bit?

@TIHan

This comment has been minimized.

Contributor

TIHan commented Nov 13, 2018

Yes, I'm now going to start looking at this.

@TIHan

This comment has been minimized.

Contributor

TIHan commented Nov 13, 2018

heejaechang#10 @heejaechang here are those sourcecodekind fixes

@TIHan

This comment has been minimized.

Contributor

TIHan commented Nov 13, 2018

With these changes, I'm confident this is all that is required for us :) Thank you!

@heejaechang heejaechang changed the title from [WIP] Misc analyzer to support semantic errors for script files in misc projects. Nov 13, 2018

@@ -277,15 +277,28 @@ public void LogAnalyzerCountSummary()
ResetDiagnosticLogAggregator();
}

private void ResetDiagnosticLogAggregator()
public static async Task<IEnumerable<DiagnosticData>> GetDiagnosticsAsync(

This comment has been minimized.

@heejaechang

heejaechang Nov 13, 2018

Contributor

this is actual new code. all other change in the engine is just making existing method static

This comment has been minimized.

@mavasani

mavasani Nov 14, 2018

Contributor

Is this method only invoked from DefaultDiagnosticIncrementalAnalyzer? It seems to be the case as we are defaulting lot of things such as includeSuppressedDiagnostics: false, logAggregatorOpt: null. If so, would it make sense to actually move this method to DefaultDiagnosticAnalyzerService?

This comment has been minimized.

@mavasani

mavasani Nov 14, 2018

Contributor

Seems even spanOpt: null, so we should either move the method to default analyzer service or make this parameters such that the callers in DefaultDiagnosticIncrementalAnalyzer pass in default values as arguments.

This comment has been minimized.

@heejaechang

heejaechang Nov 16, 2018

Contributor

if you are okay with diff, I can move all these static methods to its own static helper types or move in to analyzerhelper type. I didn't just because that will make review harder. since you already looked, I am fine to move all these to AnalyzerHelper.

This comment has been minimized.

@heejaechang

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Nov 16, 2018

Contributor

It's less about having a diff, and more about living on the right objects. Being static methods floating around somewhere doesn't make a lot of sense to me. It's unclera why we wouldn't just have these be normal instance methods on the right diagnostic analyzer subclass.

@@ -394,7 +396,7 @@ private ProjectInfo CreateProjectInfoForDocument(string filePath)
var documentInfo = DocumentInfo.Create(
documentId,
filePath,
sourceCodeKind: parseOptionsOpt?.Kind ?? SourceCodeKind.Regular,
sourceCodeKind: parseOptionsOpt?.Kind ?? (string.Equals(fileExtension, languageInformation.ScriptExtension, StringComparison.OrdinalIgnoreCase) ? SourceCodeKind.Script : SourceCodeKind.Regular),

This comment has been minimized.

@heejaechang

heejaechang Nov 14, 2018

Contributor

make sure we let F# or any other language to set document kind as script if they want through "ScriptExtension" they already provide to us. previously, we only let us to set document script in misc project.

This comment has been minimized.

@jasonmalinowski

jasonmalinowski Nov 14, 2018

Member

Can we just extract the SourceCodeKind out to a variable and set that in the "if script" block above? Maybe restructure this as

if (fileExtension == languageInformation.ScriptExtension)
{
    kind = SourceCodeKind.Script;
    if (parseOptions != null && compilationOptions != null)
    {
        // do the C#/VB specific stuff
    }
}

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Nov 14, 2018

Contributor

👍 This code is too complex.

This comment has been minimized.

@heejaechang

heejaechang Nov 16, 2018

Contributor

sure.

@heejaechang

This comment has been minimized.

Contributor

heejaechang commented Nov 14, 2018

@amcasey @mgoertz-msft is typescript or xaml care about this change? basically, when a file is opened outside of project context, when that file is added to misc roslyn project, we check whether its file extension is same as "script extension" provided to us, and if it is same, we set sourcekind of the document as script.

if you guys don't use our misc workspace, then it is not relavent.

@heejaechang

This comment has been minimized.

Contributor

heejaechang commented Nov 14, 2018

@TIHan do you want it for preview2 or master?

@heejaechang

This comment has been minimized.

Contributor

heejaechang commented Nov 14, 2018

@TIHan

This comment has been minimized.

Contributor

TIHan commented Nov 14, 2018

@heejaechang this should be for preview2 as I want my single file changes to work in preview2

@heejaechang heejaechang changed the base branch from master to dev16.0-preview2 Nov 14, 2018

@amcasey

This comment has been minimized.

Member

amcasey commented Nov 14, 2018

@heejaechang I think TS creates a regular roslyn project for its misc files but it would be good to validate the change since loose files are an important scenario for JS/TS.

@heejaechang heejaechang force-pushed the heejaechang:miscAnalyzer branch from 456d4c4 to d8693fb Nov 14, 2018

@TIHan

This comment has been minimized.

Contributor

TIHan commented Nov 14, 2018

The change seems to work very well for F#. The only thing left is change propagation after a conversation with @heejaechang . Basically, when Script1.fsx references Script2.fsx and when you change Script2.fsx it should also kick off analyzers for Script1.fsx. Currently that does not happen, but as soon as you make a change in Script1.fsx it kicks off analyzers and everything is corrected.

@heejaechang

This comment has been minimized.

Contributor

heejaechang commented Nov 14, 2018

right now, every file opened in misc workspace is under its own project. and projects in workspace doesn't have dependencies each other. that's why solution crawler doesn't know what other projects needed to be re-analyzed when a file is changed.

2 options. either FSharp explicitly ask what project needed to be re-analyzed. or for misc workspace, we always re-analyze every projects in the workspace.

but also open to other options like exposing some kind of service from F# that let us know relationship between projects in misc workspace and etc.

/// <summary>
/// Include script semantic errors
/// </summary>
ScriptSemantic = 0x04,

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Nov 14, 2018

Contributor
  1. needs documentation to explain what this mean.

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Nov 17, 2018

Contributor

I would consider this blocking. I do not see a customer need for this value.

@@ -82,7 +82,7 @@ public void RegisterLanguage(Guid languageGuid, string languageName, string scri

internal void StartSolutionCrawler()
{
DiagnosticProvider.Enable(this, DiagnosticProvider.Options.Syntax);
DiagnosticProvider.Enable(this, DiagnosticProvider.Options.Syntax | DiagnosticProvider.Options.ScriptSemantic);

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Nov 14, 2018

Contributor

needs documentation.

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Nov 18, 2018

Contributor

how is this tested?

@@ -277,15 +277,28 @@ public void LogAnalyzerCountSummary()
ResetDiagnosticLogAggregator();
}

private void ResetDiagnosticLogAggregator()
public static async Task<IEnumerable<DiagnosticData>> GetDiagnosticsAsync(

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Nov 14, 2018

Contributor

can we use an ImmutableArray here? Task<IEnumerable> is generally quite scary as we've had bugs in the past where there enumerable was accidentally made lazy, which then impacted the final code enumerating it.

diagnosticService, analyzerDriverOpt, document, analyzer, kind, spanOpt: null, logAggregatorOpt: null, cancellationToken).ConfigureAwait(false));
}

return list;

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Nov 14, 2018

Contributor

Move to ArrayBuilder, and ideally return ImmutableArray

{
DiagnosticLogAggregator = new DiagnosticLogAggregator(Owner);
// given service must be DiagnosticAnalyzerService
var diagnosticService = (DiagnosticAnalyzerService)service;

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Nov 14, 2018

Contributor

then why not type things to enforce that? i.e. don't take in an IDiagnosticAnalyzerService

{
// compiler diagnostic analyzer always support all kinds
if (HostAnalyzerManager.IsCompilerDiagnosticAnalyzer(language, analyzer))
if (hostAnalyzerManager.IsCompilerDiagnosticAnalyzer(language, analyzer))

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Nov 14, 2018

Contributor

this seems weird... we had an ownership and logic flow model that we were previously takin, but not it seems to have just become a set of random static methods on disparate types. I cannot really understand why these fnuctions are on these types.

foreach (var analyzer in analyzers)
{
list.AddRange(await Executor.ComputeDiagnosticsAsync(
diagnosticService, analyzerDriverOpt, document, analyzer, kind, spanOpt: null, logAggregatorOpt: null, cancellationToken).ConfigureAwait(false));

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Nov 14, 2018

Contributor

i saw logAggregator being passed around above, but not through here. Is that an issue? can you document in the code why it's ok to not pass this in here?

var onAnalyzerException = _owner.GetOnAnalyzerException(projectId);
onAnalyzerException(ex, analyzer, exceptionDiagnostic);
var onAnalyzerException = service.GetOnAnalyzerException(projectId, logAggregatorOpt);
onAnalyzerException?.Invoke(ex, analyzer, exceptionDiagnostic);

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Nov 14, 2018

Contributor

when does GetOnAnalyzerException return null? can we rename to TryGetOnAnalyzerException?

@CyrusNajmabadi

Ownership and control flow seems less clear than before.

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Nov 14, 2018

Is there a reason F# doesn't go the TS route? IN other words, instead of using the 'total fallback misc project' concept that Roslyn has, actually jsut define 'anonymous projects' that these files are opened together in.

When different files reference each other, you make a new project that contains that 'clique' of files. This is how we used to do things in TS (unless things have changed) if we don't have a .tsconfig or other sort of explicit project construct.

That way things just work totally normally, and there's no need to mess with all this core analyzer logic and contort things. Effectively, these are 'virtual' projects, and it can be entirely in control of F#.

This also has benefits because F# can do things like create settings/options pages that help control that project, or do whatever they want. They get all real services because this is an all-real project.

'misc' project is really the concept: we effectively don't support anything, because we're a langauge that really requires a project to function, and we don't have a project.

@heejaechang

This comment has been minimized.

Contributor

heejaechang commented Nov 14, 2018

@tmat

This comment has been minimized.

Member

tmat commented Nov 28, 2018

Or needed to be actually isolated from another workspace.

This is true for C# scripts. We don't currently have IVsHierarchy model for C# script files and we do want have them isolated from the regular C# projects. If we added virtual projects to the solution then we would probably need to carefully isolate them so that they don't affect C# features that are only supposed to operate on regular projects.

Therefore it makes sense to have them in a different workspace - e.g. in Misc Workspace. If we decide to represent them in IVsHierarchy and design and implement some kind of interaction with regular projects we could move them to VS Workspace. Regardless of whether we actually do that or not we should have flexibility in the system that allows for both approaches. That decision should be separate from whether we can run semantic analyzer on them and report diagnostics, which is what this PR is about.

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Nov 28, 2018

Regardless of whether we actually do that or not we should have flexibility in the system that allows for both approaches.

I don't actually agree with that personally. I don't find flexibility to be an defacto beneficial thing. It's often an additional tax and maintenance cost that has to demonstrate its worth versus its cost. That's why we didn't go with more 'flexible' approaches for things like TS (though we could have). It's expensive and not necessarily valuable enough to warrant it.**

As an example, we could have gone the approach that loose files didn't need hierarchies, and coudl have used the misc workspace. But, at the point where we decided we did want hierarchies, we'd now have to go through all the effort to make the misc workspace support this, thus substantially increasing the costs there. We could try to refactor all the hierarchy code out into some sort of different module that workspaces could swap in on demand, but again, more costs.

I'm fully behind the idea of reducing flexibility and providing fixed sets of components taht do their job very well and which can reduce the size of their test matrix. I think tha'ts actually healthier, and is the approach we've taken with much of our public surface area. i.e. we attempt to build components that solve the 95% case very well, often cutting out scenarios that would be costly to support, but which wouldn't add much value. This limiting and lack of flexibility can be substantively better for us.

--

** Note: this came up with practically every feature that TS/JS built. They could have all been done with more flexible approaches whereby we didn't go through the exact same codepaths as the other C#/VB features. At times it would have been super convenient to go that route. But we really wanted to avoid that as it was already complex enough (including from a testing perspective) to just make sure things were working sensibly when using the same component versus what would have been involved to use different systems.

@jasonmalinowski

This comment has been minimized.

Member

jasonmalinowski commented Nov 28, 2018

FWIW, this:

VS Workspace is tightly coupled with IVsHierarchy. I don't think it should be a requirement to represent projects/files in IVsHierarchy in order to use analyzers in a given workspace.

Isn't the case anymore post my big refactoring. VisualStudioProject intentionally doesn't even hold onto an IVsHierarchy because of the risk you might use it which breaks the free-threaded contract. Some of our legacy shims still add extra dependencies, but you can now totally add projects to VisualStudioWorkspace without an IVsHierarchy.

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Nov 28, 2018

That decision should be separate from whether we can run semantic analyzer on them and report diagnostics, which is what this PR is about.

I don't personally agree with that either, as per the above post i made. In the same way i wouldn't agree if someone came along and said "i want misc workspaces to support hierarchies, so i'm churning a lot of code to make that possible instead of just using the existing workspace that works very well for that purpose." If that were to happen, i would not think it was a separate decision to be made. It would be the core purpose of hte PR and the decision around even doing that work in the first place would be appropriate to bring up in such a PR.

Just my 2c on this.

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Nov 28, 2018

but you can now totally add projects to VisualStudioWorkspace without an IVsHierarchy.

Nice! That's very cool.

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Nov 28, 2018

Let me pop out for a second. @jasonmalinowski Is there still a good reason for the Misc workspace anymore at this point? What's the benefit of having it versus having loose files just be put into a Project that we can appropraitely tag if features care? These could show up in the Explorer-UI in some way, or not have any UI affordance at all, i don't really care :)

I'm just curious if there's an overall purpose that it's really good for at this point?

@tmat

This comment has been minimized.

Member

tmat commented Nov 28, 2018

I'm just curious if there's an overall purpose that it's really good for at this point?

I believe the isolation is still valuable. Adding special projects would just move complexity elsewhere. For example, during debugging the EnC analyzer is watching for changes in the workspace. It would need to know to ignore this special project.

@tmat

This comment has been minimized.

Member

tmat commented Nov 28, 2018

Re more flexibility vs focus on streamlined scenarios.

I'd agree if the more flexible implementation was more complex and overengineered. But in this case it is not. In this case we are removing an unnecessary assumption/dependency. it might actually make testing easier, not harder, since now we can test running semantic analyzer without mocking up the whole VS workspace.

If you only focus on streamlined scenarios too much you end up with monolithic system where every component has assumptions about rather unrelated other components. Such systems are very hard to untangle when the need arises. So there is a trade-off.

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Nov 28, 2018

since now we can test running semantic analyzer without mocking up the whole VS workspace.

I'm not sure i understand this. Why would tests need the VS workspace? AFAICT something like 99.9%+ of all diagnostic related tests don't go through the VSWorkspace at all.

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Nov 28, 2018

But in this case it is not

It seems to be to me. For example, we had to introduce an entirely new Diagnostic concept (and associated options) just to handle these types of files. We've also identified cases that this breaks down for.

If you only focus on streamlined scenarios too much you end up with monolithic system where every component has assumptions about rather unrelated other components. Such systems are very hard to untangle when the need arises. So there is a trade-off.

But in this case, we have flexibility and it feels to me that we're using that flexibility appropriately when necessary. But, when these same issues have appeared in the past, we didn't opt for such approaches, and the results worked out well. I'd much rather have a very strong and compelling case to not just use the standard existing infrastructure that already works well here.

@jasonmalinowski

This comment has been minimized.

Member

jasonmalinowski commented Nov 28, 2018

What's the benefit of having it versus having loose files just be put into a Project that we can appropraitely tag if features care?

That's all it's really doing these days, it's not really a maintenance burden. And as you observed, there's enough different behaviors around the supported change types, tweaks to services, etc. that actually makes it feel very natural per the workspace design.

@CyrusNajmabadi

This comment has been minimized.

Contributor

CyrusNajmabadi commented Nov 28, 2018

Alright, if this really is the riht way to deal with loose files, then i guess you guys can go ahead with it.

My recommendation would be then to move all the loose file cases onto it to ensure that it's really sufficient for all the needs here (i.e. TS/JS as well as F#). I'd also still push back on having specialized options for how things like semantic analysis works for this workspace. There doesn't seem to really be anything special about loose files in this regard, so having the analysis engine have to get special information from the MiscWorkspace in order to work properly seems somewhat busted to me.

heejaechang added some commits Nov 29, 2018

refactor and clean up diagnostic engine code so that it can be re-use…
…d stand alone.

code refactored is related to properly set up CompilationWithAnalyzer and compute and return diagnostics
made default diagnostic analyzer service to share code with VS diagno…
…stic analyzer service on computing diagnostics.

unlike VS diagnostic analyzer, the default one doesn't persist data or calculate diagnostics per a analyzer to improve per analyzer freshes nor compute and maintain per project analyzers and etc.

also, this add ability to calculate semantic diagnostics for script file.
enable semantic diagnostics for script file for misc workspace.
now, C#/VB script files in misc workspace will get semantic errors as well as syntax errors.

any language such as F# if document is added to misc workspace with SourceCodeKind.Script, they will automatically get semantic errors as well.

this PR doesn't address changes propagations for #load which was never supported for existing C#/VB script support.

@heejaechang heejaechang force-pushed the heejaechang:miscAnalyzer branch from 7077823 to 73f242e Nov 29, 2018

@heejaechang

This comment has been minimized.

Contributor

heejaechang commented Nov 29, 2018

@tmat restructured the commit as requested.

@heejaechang

This comment has been minimized.

Contributor

heejaechang commented Nov 30, 2018

retest windows_release_vs-integration_prtest please


// document that doesn't support compiler diagnostics such as fsharp or typescript
return _service._analyzerService.GetDiagnosticAnalyzers(document.Project);
}
}

public void RemoveDocument(DocumentId documentId)
{
// a file is removed from misc project

This comment has been minimized.

@tmat

tmat Dec 3, 2018

Member

// a file is removed from misc project [](start = 15, length = 39)

Is this comment still relevant? This shouldn't have any assumption of misc workspace, correct?

This comment has been minimized.

@tmat

tmat Dec 3, 2018

Member

Is ScriptSemantic missing here? If not I'd add a comment why.


In reply to: 238470136 [](ancestors = 238470136)

// what the full set of information is except when the file is script code.
return projectInfo.WithHasAllInformation(hasAllInformation: sourceCodeKind == SourceCodeKind.Script);

SourceCodeKind GetSourceCodeKind()

This comment has been minimized.

@tmat

tmat Dec 3, 2018

Member

GetSourceCodeKind [](start = 27, length = 17)

I don't think local function makes the code better. If you look at the function in isolation you don't know what the values of the captured locals are supposed to be. I'd either make it a static method or inline the code.

This comment has been minimized.

@heejaechang

heejaechang Dec 5, 2018

Contributor

sure

@heejaechang

This comment has been minimized.

Contributor

heejaechang commented Dec 5, 2018

@tmat addressed your feedback

@tmat

tmat approved these changes Dec 5, 2018

@heejaechang

This comment has been minimized.

Contributor

heejaechang commented Dec 6, 2018

tagging @jinujoseph

@heejaechang heejaechang merged commit c64963b into dotnet:master Dec 6, 2018

3 checks passed

license/cla All CLA requirements met.
Details
roslyn-CI #20181205.4 succeeded
Details
roslyn-integration-CI #20181205.4 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment