Type finding in DLLs and reference additions is unintuitive/brittle

[NickCraver]

Visual Studio Version: 16.2 Preview 3

Summary:
When using IntelliSense to find a type (first thought to be the result of a missing using), Visual Studio helpfully finds the DLL. However, the result of this operation is probably not one we'd ever desire to happen: a direct DLL reference added to the .csproj.

Steps to Reproduce:

1. Add a type from an non-referenced package. I was adding IMemoryCache.
2. Ctrl+. on the not-found IMemoryCache
3. Select "Add Reference to 'Microsoft.Extensions.Caching.Abstractions.dll'" from the fixes list.

Expected Behavior:
Ideally, this would offer to add <PackageReference Include="Microsoft.Extensions.Caching.Abstractions"... /> instead, or suggest <FrameworkReference />.

Actual Behavior:
A direct DLL reference is added to the .csproj:

<ItemGroup>
  <Reference Include="Microsoft.Extensions.Caching.Abstractions">
    <HintPath>..\..\..\..\Program Files\dotnet\packs\Microsoft.AspNetCore.App.Ref\3.0.0-preview6.19307.2\ref\netcoreapp3.0\Microsoft.Extensions.Caching.Abstractions.dll</HintPath>
  </Reference>
</ItemGroup>

User Impact:
It works...but in a very brittle, undesirable way. The app would very likely break when deployed.

Screenshots:

cc @davkean

[jmarolf]

all the code for this lives on https://github.com/dotnet/roslyn. We should ensure that framework references work over there for these refactorings

[davkean]

Adding @nguerrera.

[nguerrera]

Can someone point me to how this code doesn't offer to add raw references to files in the nuget global packages folder? I would expect this assembly to be seen by Roslyn in a way that is very similar to how it would see a path to a nuget assembly. In both cases, it needs to never try to add it as a plain Reference + HintPath. So the solution for nuget will probably give some insight into the solution for .NET Core targeting packs.

@CyrusNajmabadi @jasonmalinowski

Ideally, this would offer to add <PackageReference Include="Microsoft.Extensions.Caching.Abstractions"... /> instead,

I would think this would happen already if this feature is on, which I believe is default in 16.3:

or suggest <FrameworkReference />.

This would be good, but might be too much work for 16.3. If so, we should prioritize not suggesting the raw Reference (bug) over suggesting FrameworkReference (missing feature).

How does Roslyn know the mappings to all framework assemblies for the feature to suggest adding framework references? We could maybe add info there about which FrameworkReferences to pull in instead of which Reference for .NET Core 3.0+.

[jasonmalinowski]

So I don't think what @NickCraver is running into is related to the feature @nguerrera is circling. We have a general feature that implements what VB.NET had (possibly predating NuGet) where if a project A references Foo.dll, and the compiler in B says it's missing a reference to Foo.dll, we notice that project A has a reference to Foo.dll and adds it. I forget if it has special checks to exclude adding references to NuGet packages directly or not.

The circled feature by @nguerrera instead uses a hard-coded index of top packages from nuget.org to provide suggestions.

[nguerrera]

The circling is in response to the specific sentence quoted above it. What I meant there was that given that this particular API is also available in a nuget package, then the circled feature should be able to find it. I'm aware that the circled feature and the feature putting the bad reference are different. I meant if the other feature didn't do anything, then the nuget feature could still find it and add a PackageReference. However, this is limited to a subset of framework API that are also available in nuget packages. Microsoft.Extensions.* are like this.

Then the next quoted part was about having Roslyn insert FrameworkReferences, which is somewhat like the feature just above the one I circled (these are framework assemblies, but they come in groups in the project file), but we would need Roslyn to know somehow which FrameworkReference to add for which API, which I'm sure will require some design.

Let's first focus on fixing the bug:

I forget if it has special checks to exclude adding references to NuGet packages directly or not.

Can you investigate and find out what it does for that? Again, the fix for that and the fix for this might be very similar.

[CyrusNajmabadi]

I'm a bit confused by this. Afaik Roslyn was only told about the dll. We then scanned it, saw there was a matching type, abs then offered to add the same dll to the other project.

Roslyn should only be using the currency that was used with it. So if you'd originally came from nuget, but was translated into a dll for Roslyn, then I would expect the reverse translation to happen when Roslyn talks about this dll to the project.

[nguerrera]

Is this "reverse translation" what happens for nuget-acquired dlls? I would expect Roslyn to see both nuget dlls and dlls from these .NET Core targeting packs in much them same way. In either case, it would be a bug to add a raw reference with a hint path. So does the same bug exist for nuget-acquired dlls? And if not, specifically how is it avoided in the nuget case so that we can also avoid it in this case.

[CyrusNajmabadi]

Ideally, this would offer to add <PackageReference Include="Microsoft.Extensions.Caching.Abstractions"... /> instead, or suggest .

What are those reference types? I'm looking at the object model and I don't see anything that corresponds to them.

What's the recommended way for a feature to know where these dlls came from, and how should it add/update the workspace to roundtrip things.

@jasonmalinowski?

[CyrusNajmabadi]

how is it avoided in the nuget case so that we can also avoid it in this case.

For nuget, we wrote our own index that we pull down from the internet. We also specifically communicate through nuget apis to add the reference.

The references we add though have no relation to the other references you have in the project. They're all from what we got from scraping nuget

[nguerrera]

Suppose I have the following

• Project A uses Package P
• Package P is not in the index for the nuget suggestion feature
• Project B doesn't use P yet, but user types something that would be resolved if P.dll were referenced

How does Roslyn know not to suggest/add the following:

<Reference Include="P">
  <HintPath>..\..\..\Users\nicholg\.nuget\packages\p\lib\net45\P.dll</HintPath>
</Reference>

This is directly analogous to the bad reference in the issue, and this is what I've been trying to ask all along.

[CyrusNajmabadi]

Basically, there are two feature. One says: something else within this same solution could help this project. So I'll offer to add it for you. It only uses the currency we know about from the project itself.

The other feature says: lot me get information about stuff outside your project from nuget. If I find something I will offer to add it. Because it's known to be anuget package, we know to add it in that fashion.

This is the first time I've heard about the problem with the first feature. It definitely feels fishy that Roslyn is told about a dll for one project, but then it's not supposed to add that dll to another project.

Thanks!

[nguerrera]

At this time, I would not expect Roslyn to be told about P.dll from a PackageReference in my example above differently than Microsoft.Extensions.Caching.Abstractions.dll coming from FrameworkReference in @NickCraver's example. Ultimately, isn't Roslyn told about all references by their paths?

If the solution for the nuget case is that Roslyn is "told" something different than just a path, who tells it that difference and how?

[davkean]

@CyrusNajmabadi Dll isn't always the reference boundary, sometimes there's a higher abstraction that should be referenced instead.

[davkean]

Here's the code that does the same check for packages: 95a2a5e.

[davkean]

Which was fixed in response to this bug: #8686.

[nguerrera]

Bingo! Thanks, @davkean

[nguerrera]

The morally equivalent heuristic to ignore .NET Core Targeting Pack assemblies would be to also ignore if there's a 'packs' component to the path like 'packages' for nuget.

I wonder if we could have a more robust communication with SDK / Common Targets / Project System / Roslyn for this. Is there the same bug for assemblies resolved from AssemblyFolders or Extension SDKs, etc.?

[davkean]

@nguerrera I believe for raw files coming from AssemblyFoldersEx and Framework, the legacy project system will not burn in hint paths. We have a bug against us to do the same. Extension SDKs, COMReferences, etc, however, I would expect suffer from the same problem.

[nguerrera]

@jinujoseph Why was the Reoslution-Duplicate tag applied here? Also, can we triage this for 16.3? Adding that reference is pretty severe. I think it has the potential to burn .NET Core 3 customers. Things will compile on the current machine only, and not run.

I think there are essentially 3 distinct issues here:

1. Suggesting/Adding a reference with explicit HintPath to .NET Core SDK targeting pack assemblies should never happen (BUG)
2. Improving the mechanism by which Roslyn can avoid this class of bugs more generally than heuristics like ignoring 'packages' or 'packs' folders. (likely pre-existing bug for COMReference, Extension SDKs, etc.)
3. Suggesting/Adding FrameworkReference where possible for optional .NET Core components that aren't referenced yet (missing feature)

I am only asking for (1) to be triaged for 16.3. I believe a good enough tactical fix for 16.3 is to extend the 'packages' heuristic to include 'packs', which is straightforward. If you agree, and you'd like, I can offer to submit a PR that does that.

[CyrusNajmabadi]

What are FrameworkReferences? How do they differ from a dll? Thanks!

[nguerrera]

See dotnet/designs#50

TLDR: They are new for .NET Core 3. They represent a group of assemblies that are part of the framework, but optional (ASP.NET, WindowsForms, WPF).

Generally speaking, it would be good if this feature were not subject to breakage when a reference abstraction is added/changed. PackageReference is such an abstraction, and it was fixed with a heuristic around package folder names. COMReference and Extension SDKs are other pre-existing abstractions like this and probably broken.

[CyrusNajmabadi]

Ok thanks.

1. If one project references this new 'FrameworkReference' can we add that 'FrameworkReference' to another project? Are there any restrictions on that sort of thing?
2. How does one tell if something is a framework reference?

@jasonmalinowski how are these all passed to roslyn? as just dlls? or as something richer?

[jinujoseph]

@sharwell thought there was a similar bug which we were planning to dupe against. (removing the tag for now till that happen

[sharwell]

This is closely related to #34642, if not a duplicate. In that bug, I found that the addition of a direct reference only occurred if another project in the same solution already has a direct reference to the same assembly. If it's a bug to directly reference the assembly, then the bug was caused by the user in another project and not a bug in Roslyn itself.

The 'packages' heuristic should not be necessary and is fragile (it fails to handle other package folder names, even though it's a user-configurable setting), and we should definitely not try to do something similar here. We should instead perform the assembly-to-package mapping described in #34642, and fall back to directly referencing the assembly if it fails.

[CyrusNajmabadi]

The final state told roslyn to use those paths to analyze/compile this project, which is correct. It did not tell roslyn to copy/paste the paths into other projects, which is not.

At one point in time it very much was. When we were originally writing features like add-using and whatnot, these were the abstractions that were designed in close concert with the IDE/project system.

Later on new abstractions were added but i don't recall any meetings with IDE to discuss what they were, what the user experience should be around them, and how it would affect features.

Note: this affected more than just what goes in the project, it also affected the language itself. For example, there were teams that wanted 'add using' disabled because in their model of how C# was used they didn't want people manually adding 'usings', but would instead inject it themselves.

When the model of the world changes, i think it would be really good to involve relevant teams (including those building IDE features for customers). If the new model needs a different user experience, then that needs to be brought up early on so that we can appropriate design and coordinate between the teams.

For example, in this case, it looks like the belief was "oh, we'll just translate this higher abstraction to a lower one that hte later clients are used to. it will all work fine if we do that". There wasn't enough thought given to the idea that the later clients don't just receive data, they also send it back and explicitly have features to allow users to work with that lowered data.

Another example of where i assume this would affect people is that Roslyn languages (like VB) allow users to remove references that are unused. I assume this probably also breaks as we will suggest removing dlls that are part of 'packs' that probably can't be broken up.

So, IMO, it's not sufficient to just lower to a form that a later module needs. If modules can be bidirectional, there needs to be thought, design and communication with teh appropriate teams to ensure that the operations in both directions make sense.

[CyrusNajmabadi]

I only suggested starting with extending the heuristic when this was first triaged to "backlog". I suspect that getting this fully right is going to take more time than 16.3 would allow. It might involve project system, msbuild/sdk, and roslyn changes. If we think we can do something bigger in 16.3, that would be even better, of course.

This works for me. I would take the expedient route for 16.3 of simply adding "packs" to the blacklist. For post-16.3 a cleaner solution can be designed where there is an actual way of communicating the relevant information up/down the stack.

This would be my definite preference on how to proceed here.

[nguerrera]

At one point in time it very much was. When we were originally writing features like add-using and whatnot, these were the abstractions that were designed in close concert with the IDE/project system

Later on new abstractions were added but i don't recall any meetings with IDE to discuss what they were, what the user experience should be around them, and how it would affect features.

Fair. FWIW, my time doing build work is limited to the last three years.

FrameworkReference was built following a very similar implementation to PackageReference in this respect. Likewise, I can argue the other way: I don't recall a single meeting where anyone said, "Hey, the way PackageReferences are lowered to paths causes problems for the IDE, let's work together on fixing that." There's a commit from 3 years ago that applied a heuristic workaround, yet I don't recall anybody following up with us to make changes that would prevent the need for such a hack. Heck, the implementation of PackageReference follows directly from the earlier NuGet build implementation from project.json, which I believe was implemented by an IDE team member! Also, IDE veterans were actually involved in the design phases of FrameworkReference...

I don't think anyone is intentionally excluding anyone else or intentionally assuming anything, it's just that the complexity builds up over the years and it's very easy for things to get lost in translation. Teams turn over, tribal knowledge gets lost, and sometimes no amount of meetings can replace finding out that something doesn't work the hard way.

I've certainly learned a bunch here that can be applied to future similar work, and I'm very happy to collaborate on a design for 16.4+ to make this fully robust. Thanks.

[nguerrera]

Another example of where i assume this would affect people is that Roslyn languages (like VB) allow users to remove references that are unused. I assume this probably also breaks as we will suggest removing dlls that are part of 'packs' that probably can't be broken up.

Interesting. There would be like 75 unused references in a File -> New Project. I haven't seen a report of this happening, but it certainly would be a problem. How do I trigger this feature to check that it is working correctly? And if it is working correctly, maybe there's some insight from how it does so into a better solution for the suggest/add case?

[CyrusNajmabadi]

How do I trigger this feature to check that it is working correctly?

Hrmm... I could have sworn it was under VB's project properties. But i'm not sure where the cheese has moved to. @jasonmalinowski do you know where VB's "remove unused references" feature is now?

[CyrusNajmabadi]

So, it used to be here:

But i don't have that in my VS. Did we remove this from the free versions and make into an enterprise feature only?

[nguerrera]

I don't have it in a .NET Core project or a .NET Framework project, and I have Enterprise.

[nguerrera]

FWIW, that dialog isn't showing any of the dynamic references that come from its FrameworkReference, so presumably if the feature were still there, it wouldn't be trying to remove things that aren't in that list of References?

[heejaechang]

so, are we talking about a way to find out a "pack?" where a dll came from?

basically, if we find a dll is not used anymore or we need to reference a dll from project A, we have a service that we can ask what "pack" the dll came from? which can let us know whether it is plain dll reference, nugetpackage reference, or framework reference?

or are we talking about even higher abstraction where we can ask a service to either delete/add reference to the "pack" the given dll belong to?

[CyrusNajmabadi]

My idea would be:

1. be able to ask, given a Roslyn MetadataReference: "what higher level abstraction do you belong to?"
2. be able to ask: "can i add this higher level abstraction to this project?"
3. be able to actually request that the higher level abstraction be added.

All three seem necessary. The second is so we can even know if we can show the user an option. This is often an issue for us in the new complex proejct world since so many types of things for one project are not allowed for another. Then, if we've disovered that it is ok, and we've shown the user, and they've selected things, we want to be able to actually perform the operation.

Note: ideally all the querying can be done without exceptions being thrown. We have tried to the above for nuget. However, even attempting to ask questions can throw exceptions, which is highly annoying and leads to tons of watson hits which we then need to disable.

[heejaechang]

yep. my idea is same as @CyrusNajmabadi for add using. but if we are going to have such service, I would hope it to also support remove for remove reference features.

[nguerrera]

I confirmed that this is broken for COMReference, which I believe has been around since VS 2005, or as long as MSBuild. 😉

In that case, it tries to reference a temporary .dll in obj of another project. This creates a race condition in the build.

[CyrusNajmabadi]

What is a COMReference? (I totally find it lamentable that i was working in the C# IDE space since VS2002 and i've never even heard of that until now). :)

[nguerrera]

It's what you get when you use Add Reference -> COM

It lets you reference a COM type library and handles the generation of the interop assembly for you.

https://docs.microsoft.com/en-us/dotnet/framework/interop/how-to-add-references-to-type-libraries

[CyrusNajmabadi]

Note: teh fact that no one seems to have ever reported an issue here makes me feel like maybe we're totally fine with existing behavior (or at least, don't have to fix it with high priority) for 16.3. I totally agree the behavior we have is not desirable and can pit-of-fail someone. But i guess it's unclear to me how bad it actually is and if we can just accept that for 3 months while working on a better fix later.

That said, the cheapo fix of just adding "pack" to "packages" in our roslyn check seems so easy to do, that we might as well take it.

[nguerrera]

I agree on the lack of urgency for COMReference if nobody has reported it. FrameworkReference is used by every .NET Core 3 project so this is why I would like that one to not have this issue from the outset. .NET Core 3 makes its RTM debut in 16.3.

I also thought it was interesting that the idea of such an abstraction is actually quite old.

[jasonmalinowski]

Explaining the history: VB did have the button prior to Roslyn, but the feature was removed when we shipped Roslyn 1.0. We actually forgot about the feature until somebody filed the bug saying the button didn't work. 😄 It might be worth revisiting and looking to add such a feature, but in a NuGet world things are of course tricker (but not insurmountable.)

[jinujoseph]

Design Meeting Notes

For 16.3
Add /pack & NugetFallback folder into the skip list.

For future

Roslyn should not be in the business to decide if the addreference should be offered or not , it will be decided by project system defined here dotnet/project-system#5275