Fsharp: Wrong #r and #load directives are not reported in syntax/type check results errors

[auduchinok]

#load "qwerty"
#r "blah blah"

Examples from the Rider issue:

[dsyme]

@auduchinok Yes, this is some kind of difference in reference processing between your use of FCS and FSharp.Compiler.Private (as used by VF#)

• FSharp.Compiler.Service.dll doesn't use MSBuildReferenceResolver, which we regard as legacy. That might be what's reporting the better errors. We should adjust SimulatedMSBuildReferenceResolver to give better error messages I suppose, assuming that's being used.

• It's possible the script --> options process you're using is swallowing errors somehow.

It would be good to have a repro in terms of an FCS test case that fails to report errors.

[auduchinok]

@dsyme From my understanding it may be connected to error logging phases/filtering as it is also reproduced for #load directives where the resolver isn't used.
We've switched to using SimulatedMSBuildReferenceResolver in recent nightlies but even when we used MSBuildReferenceResolver these errors weren't reported.

• It's possible the script --> options process you're using is swallowing errors somehow.

I've tried it in Ionide and didn't get the errors as well. Either the both Rider and Ionide somehow swallow the errors or the problem is in FCS indeed. I've looked at how we get options in debugger and didn't see them appear anywhere.

[dsyme]

@auduchinok Yes, it's possible that FCS is swallowing errors, or possible you're ignoring errors that it is reporting. Could you put together a test case that summarizes the calls you're making to FCS in this case?

[auduchinok]

@dsyme I've added a failing test for this case.

What I've found is both Rider and Ionide filter a reference to FSharp.Core.dll that comes from GetProjectOptionsFromScript, as IIRC it could come from GAC and make FCS fail later, and replace it with the one from Reference Assemblies folder or some other place where FSharp.Core.dll is known to be valid for FCS. I also remember having some problems on Mac/Mono with the provided reference, will try to reproduce and look into.

I suspect passing different project options to ParseAndCheckFileInProject creates another incremental builder which doesn't have these errors. Passing unmodified options provided for the script (so existing builder is used) makes these errors appear in type check results.

Probably there's no need in modifying project options since MSBuildReferenceResolver isn't used anymore and SimulatedMSBuildReferenceResolver may return correct FSharp.Core.dll path. I'll look into it.

[auduchinok]

I removed replacing FSharp.Core.dll references and didn't have any issues for scripts and got all the expected errors for hash directives.
Should FCS provide these errors when a new builder is created for options that were created by a client?
Should there be a way to specify FSharp.Core path in GetProjectOptionsFromScript call if needed for some reason?

[dsyme]

Should FCS provide these errors when a new builder is created for options created by a client?

Well, I believe we should always report back any errors from getOrCreateBuilderAndKeepAlive. Perhaps the second options have you're passing in have filtered out the bad assembly references?

Should there be a way to specify FSharp.Core path in GetProjectOptionsFromScript call if needed for some reason?

No, I don't think so. The FSharp.Core reference (and any other references) should be the "right" ones and if they aren't then we need to work out why. I suspect the previous problems may have been to do with the optdata/sigdata files being missing, but now they are always in FSharp.Core.dll so that should be less of a problem now.

[KevinRansom]

@auduchinok is this PR still needed or can it be closed?

[auduchinok]

@KevinRansom We've managed to overcome this issue by using exact project options produced by the checker for the script. Previously the errors were lost due to us modifying the options, which lead to another background builder creation which in turn made us not receive the initial builder creation errors.
So there's still an issue about losing the errors of modifying the options but there's a known workaround as well. You may close the issue if you think it's not needed anymore.

[baronfel]

I can reproduce this in FSAC (with FCS 32, so ~1 month old). We have to modify the projectoptions returned by FCS for a few reasons:

• if users have specified FSI arguments we supply augment the projectoptions with them (EDIT: @auduchinok showed me that we can supply custom options to the GetProjectOptionsFromScript call via the otherFlags parameter, so this point is not relevant)
  
  
  
  • this can impact parsing/checking due to things like LangVersion
  
  
• if users are targeting a version of .net sdk other than the one that dotnet is currently running with
  
  
  
  • this is related to the issue I made around framework reference sets
  
  
  • this needs to be validated against FCS 33
  
  
• the assemblies returned in the options from FCS don't contain a reference to mscorlib.dll and something in typechecking requires this
  
  
  
  • without it you get the following error:
    
    [Checker] ParseAndCheckFileInProject - /Users/chethusk/oss/scratch/bad-r-hash.fsx completed with errors [unknown (1,1)-(1,1) parameter error Assembly reference 'mscorlib.dll' was not found or is invalid]
  
  

So it's not really feasible for us to not modify project options.

For FSAC, it's possible to work around this because erroneous #r and #l directives are detectable in the project options:

UnresolvedReferences =
  Some UnresolvedReferencesSet
  [UnresolvedAssemblyReference
     ("blah blah",
      [AssemblyReference
         (/Users/chethusk/oss/scratch/bad-r-hash.fsx (1,0--1,14) IsSynthetic=false,
          "blah blah",None)])]
OriginalLoadReferences =
  [(/Users/chethusk/oss/scratch/bad-r-hash.fsx (2,0--2,25) 
    IsSynthetic=false, 
    "/Users/chethusk/oss/scratch/nothing/here.blah")]

So we can probe these locations and create additional diagnostics based on:

• keys present in the UnresolvedReferencesSet, and
• OriginalLoadReferences that do not actually exist/are not fs/fsx files.

I don't want to do that though because it brings us out of sync with what the compiler sees.

[KevinRansom]

I think fcs has this handled somehow. So I am closing this down. Please re-open if we determine what we need to change.

Thanks for this contribution.

Kevin