Fixes issue #6061 Add using writes the wrong reference down for a #6228
Conversation
|
I'm not sure in which use case this service is used - @Therzok can you provide one ? The VS.NET implementation loads reflection assemblies and checks if the type exists - even if the name is null. I would really like to avoid that - looks like a double check. Roslyn has some way of discovering the type -> assembly name therefore I think this check assembly name -> type is not needed. However if it is I would rather use cecil to look up the name - loading assemblies that way pollutes the app domain and loading may fail due to dependency/framework compatibility issues. Any comments ? |
There's 1 scenario that's fixed by this patch, and that's searching reference assemblies without having them referenced in any project. Saw it used in the codeaction, assumed that it was the fix. We currently have that toggled off, sorry for misdiagnosing. To fix the bug report scenario, we need to normalize the references at MonoDevelop workspace level and change the path when we encounter a GAC assembly: My test project to repro the issue: Open I've pushed a minor fixup to this branch, so the (currently) unused scenario is working |
| if (!(workspace.GetMonoProject (projectId) is DotNetProject monoProject)) | ||
| return null; | ||
|
|
||
| string assemblyFile = Runtime.SystemAssemblyService.DefaultAssemblyContext.GetAssemblyLocation (assemblyName, monoProject.TargetFramework); |
There was a problem hiding this comment.
Shouldn't this just use the assembly context from the project? That is, monoProject.AssemblyContext
There was a problem hiding this comment.
sounds reasonable - didn't know that projects have an assembly context. Just cnp that code from somewhere else in the IDE.
|
Can we have unit tests for this based on Marius' example? |
|
We can test the service however I don't know how this example should help - it's just using a quick fix. Better is to test such services directly there is no need for throwing quick fixes in they may change or get removed or something. Roslyn is nothing we can control. |
|
We just need to test that the service is properly integrated in VSMac. Up to you about using the example or not. |
|
@Therzok can you please verify? |
|
Issue is still not fixed, seeing relative path to It seems like roslyn is not considering the assemblies as being reference assemblies, but like simple assembly references. Trying to figure out what is causing them not to be seen as framework reference assemblies by Roslyn. |
|
Discussed with roslyn folks: https://gitter.im/dotnet/roslyn?at=5bc4c2f2e65a634336e25bdf Adding some code in workspace layer to handle this. |
|
Currently working on adding tests. Two things don't work yet in this version:
|
|
Added unit tests. The one problem remaining is I don't know how to solve that problem, especially without breaking AssemblyContext API or making it incredibly slow (IO + lots of allocations even on fast path). |
Therzok
force-pushed
the
master-issue6061
branch
from
338d251
to
9fa1eaf
Compare
|
@Therzok is the glib-sharp issue a big problem? In which cases things won't work? |
|
It writes a relative path to I'm not sure it's worth pursuing a fix for that, I've only seen glib-sharp, gtk# and co in there. |
|
Ok then. |
|
Let's resolve the conflicts and merge. |
framework assembly
implementation.
…nwith relative paths
Therzok
force-pushed
the
master-issue6061
branch
from
9fa1eaf
to
0aad4b1
Compare
|
Rebased. |
|
@monojenkins backport release-7.7 |
framework assembly