Metadata of C# (and other) Assemblies. #229
Conversation
|
FSharp.Formatting support for C# would be really nice. @7sharp9 @dsyme @vasily-kirichenko @dungpa could please take a look at this? |
|
Yes, it is important to understand better why these are needed in FSharp.Formatting. Specifically the PR would need to have test cases that exercise these new cases. Is there any chance you could extract an isolated simple repro from FSharp.Formatting's use of this API? thanks |
|
FSharp.Formatting is trying to load the assemblies: https://github.com/tpetricek/FSharp.Formatting/blob/master/src/FSharp.MetadataFormat/Main.fs#L816. But from the looks of it this is more a workaround than properly loading the dlls with an API call (is there even something like this?). Because of this setup the current compile unit (ccu?) is an FSharp project so isFSharp is true and https://github.com/fsharp/FSharp.Compiler.Service/blob/master/src/fsharp/vs/Symbols.fs#L344 is doing the rest... To get a setup like this one would either call the C# compiler or include a compiled C# dll. Are you sure you want any of those things included in this repro as unit test? Or are you asking for a simple repro in an extra git repository (which should actually not be too difficult to do, or just run FSharp.Formatting on any C# project/dll). Maybe this more straightforward to do with more knowledge of the API (which I currently do not have)? |
|
Strange, ccu.IsFSharp should be false for those DLLs |
|
thisCcu is the compiling FSharp module and the entity seems to be ERefLocal for which the ccu is None -> isResolvedAndFSharp returns true (currently hardcoded actually). My changes basically change the thisCcu pointer to point to the current dll (where IsFSharp is false like it should be). I don't know if this can do any harm, or how this design is supposed to work. |
…o get all methods in "MembersFunctionsAndValues"). This will be used by FSharp.Formatting for example.
|
I have now added a C# project and a test-case showing the issue (I ensured that the test fails without the patch). The commented lines in the test-method are not working (even with my patch), but they should eventually (imho). I hope this can bring more attention to this issue! |
| //String.IsNullOrWhiteSpace(members.["InterfaceEvent"].XmlDocSig) |> shouldEqual false | ||
|
|
||
| () | ||
| | None -> () |
There was a problem hiding this comment.
I suppose that the code should fail in this case? (If the entity was not found, then something else went wrong...)
There was a problem hiding this comment.
Correct :). While I noticed this shortly after pushing it, it's good to have a reminder comment, thanks. I guess it is no big deal for now (as that part is never executed), but I will fix that after getting an overall review/comment on the code.
|
I don't know enough about F# compiler internals, but the test certainly looks correct to me! Also +1 to providing simpler API for analyzing DLLs (so that the boilerplate code to create an empty project does not have to be repeated in F# Formatting and elsewhere). |
|
It seems wrong to include a solution test case into |
|
The Can the reference to the project and the project itself stay in the FSharp.Compiler.Service solution? Otherwise I'm not sure how to make the (new) test work by just opening the solution file and building FSharp.Compiler.Service.Tests... |
matthid
changed the title
| | _ -> true | ||
| | _ -> | ||
| let ccu = defaultArg (ItemDescriptionsImpl.ccuOfItem cenv.g (getItem entity)) cenv.thisCcu | ||
| ccu.IsFSharp |
There was a problem hiding this comment.
I think these three lines should just be a single line that changes the "true" case on line 197 to "cenv.thisCcu.IsFSharp". I think the getItem change is then no longer needed and this part of the fix becomes one line.
|
It looks good, I've made one suggestion for a code simplification |
|
Thanks for looking into this, I pushed the simplification. |
Metadata of C# (and other) Assemblies.
Better support for loading C# (and other) dlls (we need IsFSharp to be false to get all methods in "MembersFunctionsAndValues"). This will be used by FSharp.Formatting for example.
I used this with success in fsprojects/FSharp.Formatting#208 but I'm not exactly sure what I have done :)
If somebody can comment on this why it's needed or why it's wrong that would be nice. It looks like the right thing to do would be to get the ccu from the entitiy, but this seems to be impossible in this case (because it is not unresolved).