dotnet / project-system

The .NET Project System for Visual Studio
MIT License
969 stars 387 forks source link

P2P reference treated as metadata reference when ref assemblies are enabled #2478

Open jcouv opened 7 years ago

jcouv commented 7 years ago

Repro:

  1. Set up 2 .NET Core projects: a library and a client.
  2. Turn on reference assemblies in both (<CompileUsingReferenceAssemblies>true</CompileUsingReferenceAssemblies>).
  3. Instantiate a type from the library in the client code and add the project reference.
  4. In the client, use "Go To Definition" on a type from the library. This opens a "[from metadata]" window. But I expect it would open the library source.
  5. In the library, add a public API. Then try to use it from the client, I expect intellisense to pick up this new API. But the current behavior is that a Build operation is required for the new API to become visible to the client.

From discussion with Jason, this is likely a misbehavior of the project system exposing information about project references to the IDE.

Relates to:

FYI @panopticoncentral @davkean @srivatsn for triage. Can we take a look for 15.3?

jcouv commented 7 years ago

From discussion with @srivatsn, the new project system simply passes to Roslyn the same path that was given to csc.exe, then Roslyn compares that with known outputs from other project. If any of them match, the reference is rewired to a compilation reference.

What is likely happening here is that csc.exe is getting a path to the ref assembly, passing that to Roslyn, but it doesn't match any known outputs. Sri suggested there may be multiple design solutions, which likely involve project-system, Roslyn or both.

I'll let @davkean @jasonmalinowski comment on possible designs. I can set up a quick meeting if needed.

jcouv commented 7 years ago

From discussion with Dave and Jason, I think there are three options so far:

  1. have the project-system pass the output path for the ref assembly (bin/ref/a.dll path) instead of current path (bin/a.dll)
  2. have the project-system pass the ref assembly as an additional value to the existing one and have Roslyn handle that extra value
  3. use some generalized mechanism for passing values (bag of properties or other format) across from project-system to Roslyn

Note that there is an msbuild flag that lets a client project ignore the ref assembly produced by a library project. We didn't discuss how that might fit in either of those options (it probably can't fit with option (1)). I'll follow-up with Sri and Jared to get a sense of what is realistic trade-off, if any, for 15.3. My sense is that options (2) and (3) seem too late in the cycle.

jcouv commented 7 years ago

@srivatsn @jaredpar What would you think if we put option (1) in place for 15.3, just to unblock our dogfooding of the ref assemblies feature, and then followed-up with a more proper and refined solution for 15.5?

jcouv commented 7 years ago

From discussion with Sri and Jason, we're leaning towards option (2). That means leave the existing API as-is (to avoid breaking other consumers) and adding a new call to pass a list of output paths. Sri will get back to us on that option, and we'll also get @Pilchie's thoughts on that.

For the record, we discussed an option (4) where Rolsyn would do some string manipulation (remove "ref/" in the path) before converting path to project reference.

Also, Rainer confirmed that the output path for reference assemblies can be customized in MSBuild (with @(IntermediateRefAssembly)).

jcouv commented 7 years ago

From discussion with Jason and Kevin about 15.3 scope, I think we're landing on option (4) after all, which is a Roslyn-side workaround. Filed https://github.com/dotnet/roslyn/issues/20412 to track that. We should continue the discussion about full fix for 15.5. Thanks

davkean commented 7 years ago

Agreed, I'd like to introduce a relationship where we just pass a property bag over to the language service, and we just have a CPS XAML rule that decides what gets in that property bag. It's the same relationship we have with NuGet and it works really well and is cheap to add additional data.

jcouv commented 7 years ago

@panopticoncentral @davkean Just a kindly ping to make sure this is still on the radar for 15.5. The implemented workaround for patching P2P references seems to work, but it still has hardcoded logic for the "ref" folder. Let me know. Thanks

jcouv commented 7 years ago

@panopticoncentral @davkean I didn't hear back from you.

jcouv commented 7 years ago

@panopticoncentral @davkean I didn't hear back from you for two weeks.

davkean commented 5 years ago

@jcouv We've introduced this property bag and I'm ready to start passing reference assembly data across to Roslyn. From above, it's not clear what data you actually want passed. Can you clarify?

jcouv commented 5 years ago

@davkean You should probably sync up with @jasonmalinowski. Jason introduced a workaround (there is some hardcoded assumption that ref assemblies live in a folder called ref). He should confirm whether the property bag you introduced meets his needs and the workaround could be removed now.

jasonmalinowski commented 5 years ago

So all we just need is the path to the reference assembly in the bin path. I'm not sure if that's exposed via a property that would be pulled off of the MSBuild evaluation or something else, but just toss it into the property bag and we'll do it from there.

NinoFloris commented 5 years ago

I would be stoked to have this work soonish ^_^