Generated Metadata for Abstract Classes is missing the abstract class attribute

cloudRoutine commented 8 years ago

When I generate the metadata for HostWorkspaceServices

namespace Microsoft.CodeAnalysis.Host
{
    /// <summary>
    /// Per workspace services provided by the host environment.
    /// </summary>
    public abstract class HostWorkspaceServices
    {
        /// <summary>
        /// The host services this workspace services originated from.
        /// </summary>
        /// <returns></returns>
        public abstract HostServices HostServices { get; }
...

The resulting metadata is

namespace Microsoft.CodeAnalysis.Host

open Microsoft.CodeAnalysis
open Microsoft.CodeAnalysis.Host

type HostWorkspaceServices =
    new : unit -> HostWorkspaceServices
    /// Finds all language services of the corresponding type across all supported languages that match the filter criteria.
    member FindLanguageServices : filter:HostWorkspaceServices.MetadataFilter -> System.Collections.Generic.IEnumerable<'TLanguageService>
    /// Gets the Microsoft.CodeAnalysis.Host.HostLanguageServices for the language name.
    member GetLanguageServices : languageName:string -> HostLanguageServices
    /// Gets a workspace specific service provided by the host identified by the service type. If the host does not provide the service, this method returns System.InvalidOperationException.
    member GetRequiredService : unit -> 'TWorkspaceService when 'TWorkspaceService :> IWorkspaceService
    /// Gets a workspace specific service provided by the host identified by the service type. If the host does not provide the service, this method returns null.
    member GetService : unit -> 'TWorkspaceService when 'TWorkspaceService :> IWorkspaceService
    /// The host services this workspace services originated from.
    member HostServices : HostServices
    /// Returns true if the language is supported.
    member IsSupported : languageName:string -> bool
    /// A service for storing information across that can be retrieved in a separate process.
    member PersistentStorage : IPersistentStorageService
    /// A list of language names for supported language services.
    member SupportedLanguages : System.Collections.Generic.IEnumerable<string>
    /// A service for storing information in a temporary location that only lasts for the duration of the process.
    member TemporaryStorage : ITemporaryStorageService
    /// The workspace corresponding to this workspace services instantiation
    member Workspace : Workspace

It should show the abstract class attribute above the type, i.e.

namespace Microsoft.CodeAnalysis.Host

open Microsoft.CodeAnalysis
open Microsoft.CodeAnalysis.Host

[<AbstractClass>]
type HostWorkspaceServices =
...

If attributes are generally being left out of the metadata, perhaps the fix is to add them generally? If that would add far too many attributes maybe we could identify a subset of attributes that would be most useful to add.

TeaDrivenDev commented 8 years ago

As I discussed with @dungpa, I would like to start working on this as part of my FSSF mentorship, as it doesn't require touching a lot of different parts of the code.

dungpa commented 8 years ago

On this particular issue, the explanation is that we haven't handled Abstract attribute yet. We should handle it somewhere around https://github.com/fsprojects/VisualFSharpPowerTools/blob/7af929ea59b6c610a1f0ac0179ed8f40e30ba5f2/src/FSharp.Editing/CodeGeneration/SignatureGenerator.fs#L492-L509

It would be great to modify an existing test or add a seperate test to https://github.com/fsprojects/VisualFSharpPowerTools/tree/03c613acd3aa67cb5e3a339d3dcf1c7c783f30c2/tests/data/gotodef/generic-cases for the changes. The test is executed at https://github.com/fsprojects/VisualFSharpPowerTools/blob/03c613acd3aa67cb5e3a339d3dcf1c7c783f30c2/tests/FSharp.Editing.Tests/GoToDefinitionTests.fs#L154

Hope this gives you enough details to get started.

TeaDrivenDev commented 8 years ago

To verify my understanding here - is it correct that the same issue also exists for the Struct attribute? (see https://github.com/fsprojects/VisualFSharpPowerTools/blob/03c613acd3aa67cb5e3a339d3dcf1c7c783f30c2/tests/data/gotodef/generic-cases/go-to-empty-struct-metadata/input.fs and https://github.com/fsprojects/VisualFSharpPowerTools/blob/03c613acd3aa67cb5e3a339d3dcf1c7c783f30c2/tests/data/gotodef/generic-cases/go-to-empty-struct-metadata/expected.fs)

TeaDrivenDev commented 7 years ago

I have been poking around a bit and trying a few things to expand my image of the situation, and I have found out a few things:

A test case like what @dungpa said should be added already exists: https://github.com/fsprojects/VisualFSharpPowerTools/blob/03c613acd3aa67cb5e3a339d3dcf1c7c783f30c2/tests/data/gotodef/generic-cases/go-to-abstract-class-definition-with-default-members/input.fs and https://github.com/fsprojects/VisualFSharpPowerTools/blob/03c613acd3aa67cb5e3a339d3dcf1c7c783f30c2/tests/data/gotodef/generic-cases/go-to-abstract-class-definition-with-default-members/expected.fs
The test case tests for the presence of AbstractClass attribute in the expected metadata and doesn't fail.
VFPT does actually output the AbstractClass attribute in the metadata - if the class in question has been defined in F# and thus already has the attribute (I suppose this is why the test succeeds). For a class defined in C#, it will be missing, as originally reported.

So, as I understand it, the issue isn't so much handling AbstractClassAttribute, but rather adding it for an abstract class that doesn't have it. I'm currently trying to figure out how to add a test case for that as a first step.

TeaDrivenDev commented 7 years ago

I've started implementing the fix and have run into the first real obstacle:

The type being handled is passed to the writeType function as a value of type FSharpEntity. This has the problem that it doesn't contain information about whether the type is abstract (unless it is an F# abstract class, in which case the Attributes collection contains the AbstractClassAttribute, but this is the case we aren't here for).

I have tried to get around that with this:

    let assembly = System.Reflection.Assembly.ReflectionOnlyLoadFrom(typ.Assembly.FileName.Value);
    let clrType = assembly.GetType(typ.FullName)

    let abstractClassAttributeHasToBeAdded =
        (not typ.IsFSharp)
        && typ.IsClass
        && clrType.IsAbstract
        && not (hasAttribute<AbstractClassAttribute> typ.Attributes)

This is kludgy, and I'm not sure if it works in all cases, but it does work in a simple test case. It does not work, however, for types in mscorlib from a different framework version than VFPT, because even when using the reflection only context, it is not possible to load multiple different versions of mscorlib at the same time.

Ideally, we would get the System.Type corresponding to the FSharpEntity value, and @7sharp9 said there should be a way to do that, but I haven't been able to find out how.

What I have found is a mysterious Entity property on the FSharpEntity value that looks like it might be helpful - but I'm only seeing that while debugging; it's simply not present at design time. I currently don't understand at all where that is coming from.

TeaDrivenDev commented 7 years ago

So, that Entity property is apparently the EntityRef used to create the FSharpEntity, but it's hidden by the .fsi file.

My current understanding is that ideally, there should be an IsAbstract property on FSharpEntity, but that obviously would require a change to FCS.

dungpa commented 7 years ago

Sorry for the late reply (I have been travelling in the last few days).

A few points:

We should not go into reflection route unless there is no alternative. This is to separate between compile time and runtime tooling.
Ideally FCS should provide an IsAbstract member for FSharpEntity. We could send a PR if we figure out a way.
We can be sure that a C# class is an abstract class if it is not F# class (entity.IsFSharp = false) and it has an abstract member (check all of its MemberOrValue for their IsDispatchSlot properties). Do you think this is an OK idea?

TeaDrivenDev commented 7 years ago

I thought about reflection as well, and I'm thinking the same as you. Especially as according to @7sharp9, EntityRef is an internal TAST type, so we'd probably have to use reflection all the way to the IsAbstract property of ILTypeDef. That does not seem like the right thing to do.

From what I'm seeing in the debugger, I think we'd need to add this to FSharpEntity:

    member __.IsAbstract =
        isResolved () &&
        match entity.TypeReprInfo with
        | TILObjectRepr (_, _, ilTypeDef) -> ilTypeDef.IsAbstract
        | _ -> false

I'm currently trying to figure out how to build FSharp.LanguageService.Compiler.dll with that change to test it in VFPT. I can't get it to build in either VisualFSharp nor FCS, though.

We can be sure that a C# class is an abstract class if it is not F# class (entity.IsFSharp = false) and it has an abstract member (check all of its MemberOrValue for their IsDispatchSlot properties). Do you think this is an OK idea?

I considered that as well, but that would be inexact for types that are declared abstract but have no (abstract) members. That may not make much sense, but it's possible to do.

dungpa commented 7 years ago

Now I think it's best to send a PR to FSharp.Compiler.Service. We still need to add a test to FCS to check whether this works.

I'm currently trying to figure out how to build FSharp.LanguageService.Compiler.dll with that change to test it in VFPT. I can't get it to build in either VisualFSharp nor FCS, though.

Could you provide the errors you encountered? I was confused since FSharp.LanguageService.Compiler.dll, which is part of VisualFSharp code base, has nothing to do with FSharp.Compiler.Service.

TeaDrivenDev commented 7 years ago

Sorry, my mistake; I wasn't aware those are different things. I have made the change to FSharp.Compiler.Service now, compiled the DLL and referenced it in VFPT, and generating the metadata now works as expected when using the application - however, all the tests (or at least a very large number) in VFPT break with various MissingMethodExceptions in (see attached NUnit test results file). I don't quite understand why - both FSharp.Editing and FCS are .NET 4.5 and reference FSharp.Core 4.4.0.0.

As for the test to be added to FCS, where would that go, and what would it have to look like?

TestResults.xml.txt

dungpa commented 7 years ago

Looking at the test results, it seems like there are internal breaking changes in AST in FCS. Once a new FCS NuGet package is published, we should upgrade and adapt accordingly in VFPT.

For now, a test for FCS should be added to https://github.com/fsharp/FSharp.Compiler.Service/blob/7e18b8a1aa08e6d4df33bdafbd1f98743a4bb9d7/tests/service/ProjectAnalysisTests.fs

TeaDrivenDev commented 7 years ago

But why don't breaking changes in FCS cause compiler errors in VFPT or the tests?

7sharp9 commented 7 years ago

Are you using the untyped ast

On 11 Dec 2016 6:12 pm, "TeaDrivenDev" notifications@github.com wrote:

But why don't breaking changes in FCS cause compiler errors in VFPT or the tests?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fsprojects/VisualFSharpPowerTools/issues/1482#issuecomment-266297371, or mute the thread https://github.com/notifications/unsubscribe-auth/AAj7yp76_5GMhZ8Le8bOxRh4DyR8aJAhks5rHD0TgaJpZM4Kmy-5 .

dungpa commented 7 years ago

@TeaDrivenDev I don't know how you did it. If you take the new FCS and compile the whole VFPT solution against it, there will be compiler errors due to breaking changes.

TeaDrivenDev commented 7 years ago

@7sharp9 No. This happens without any changes to VFPT, just with a newly built FSharp.Compiler.Service.dll.

@dungpa I referenced the new DLL directly in the projects. I now tried to build the FCS NuGet package to get a more meaningful result, but I'm running into errors; I have logged an issue there.

fsprojects-archive / zzarchive-VisualFSharpPowerTools

Generated Metadata for Abstract Classes is missing the abstract class attribute #1482