Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GetAPI generates invalid code when interface property is implemented in a base class #2066

Closed
pakrym opened this issue Feb 20, 2019 · 1 comment
Labels
area-Infrastructure-libraries Area maintained by .NET libraries team: APICompat, AsmDiff, GenAPI, GenFacades, PkgProj, etc

Comments

@pakrym
Copy link
Contributor

pakrym commented Feb 20, 2019

Consider the following class structure:

    public class PagePropertyModel : ParameterModelBase, ICommonModel
    {
        // ...
        public PageApplicationModel Page { get; set; }

        MemberInfo ICommonModel.MemberInfo => PropertyInfo;

        public PropertyInfo PropertyInfo { get; }

        public string PropertyName
        {
            get => Name;
            set => Name = value;
        }
   }

    public abstract class ParameterModelBase : IBindingModel
    {
        // ...
        public IReadOnlyList<object> Attributes { get; }

        public IDictionary<object, object> Properties { get; }

        public Type ParameterType { get; }

        public string Name { get; protected set;  }

        public BindingInfo BindingInfo { get; set; }
    }


    public interface ICommonModel : IPropertyModel
    {
        IReadOnlyList<object> Attributes { get; }
        MemberInfo MemberInfo { get; }
        string Name { get; }
    }

    public interface IBindingModel
    {
        BindingInfo BindingInfo { get; set; }
    }

In it ICommonModel.Attributes and ICommonModel.Properties gets implemented by ParameterModelBase when PagePropertyModel inherits from it and implements ICommonModel.

GenAPI produces the following generated code:

   [System.Diagnostics.DebuggerDisplayAttribute("PagePropertyModel: Name={PropertyName}")]
    public partial class PagePropertyModel : Microsoft.AspNetCore.Mvc.ApplicationModels.ParameterModelBase, Microsoft.AspNetCore.Mvc.ApplicationModels.ICommonModel, Microsoft.AspNetCore.Mvc.ApplicationModels.IPropertyModel
    {
        public PagePropertyModel(Microsoft.AspNetCore.Mvc.ApplicationModels.PagePropertyModel other) : base (default(System.Type), default(System.Collections.Generic.IReadOnlyList<object>)) { }
        public PagePropertyModel(System.Reflection.PropertyInfo propertyInfo, System.Collections.Generic.IReadOnlyList<object> attributes) : base (default(System.Type), default(System.Collections.Generic.IReadOnlyList<object>)) { }
        System.Reflection.MemberInfo Microsoft.AspNetCore.Mvc.ApplicationModels.ICommonModel.MemberInfo { get { throw null; } }
        public Microsoft.AspNetCore.Mvc.ApplicationModels.PageApplicationModel Page { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
        public System.Reflection.PropertyInfo PropertyInfo { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
        public string PropertyName { get { throw null; } set { } }
        System.Collections.Generic.IReadOnlyList<object> Microsoft.AspNetCore.Mvc.ApplicationModels.ICommonModel.get_Attributes() { throw null; }
        System.Collections.Generic.IDictionary<object, object> Microsoft.AspNetCore.Mvc.ApplicationModels.IPropertyModel.get_Properties() { throw null; }
    }

Problem is that get_Attributes and get_Properties are not generated as properties.

I tracked the problem to TypeExtensions.GetAccessorType method: methodDefinition.ContainingTypeDefinition.Properties doesn't contain Attributes and so accessor kind detection fails.

dougbu added a commit that referenced this issue Jan 3, 2020
- #2066
- handle cases where accessors are overridden but related events or properties are not
  - use method prefixes rather than focus on events and properties defined in current type
@dougbu dougbu mentioned this issue Jan 3, 2020
dougbu added a commit that referenced this issue Feb 18, 2020
- #2066
- handle cases where accessors are overridden but related events or properties are not
  - use method prefixes rather than focus on events and properties defined in current type
dougbu added a commit that referenced this issue Mar 24, 2020
- #2066
- handle cases where accessors are overridden but related events or properties are not
  - use method prefixes rather than focus on events and properties defined in current type
dougbu added a commit that referenced this issue Mar 31, 2020
- #2066
- handle cases where accessors are overridden but related events or properties are not
  - use method prefixes rather than focus on events and properties defined in current type
dougbu added a commit that referenced this issue Apr 2, 2020
- #2066
- handle cases where accessors are overridden but related events or properties are not
  - use method prefixes rather than focus on events and properties defined in current type
dougbu added a commit that referenced this issue Apr 2, 2020
- Do not emit fake `[StructLayout]` attributes matching defaults
  - see #3144 discussions (not an exact fix for that request)
  - `LayoutKind.Auto` is _not_ the default for value types
  - nit: sort `using`s

- Improve discovery of accessors
  - #2066
  - handle cases where accessors are overridden but related events or properties are not
    - use method prefixes rather than focus on events and properties defined in current type

- Add GenAPI `--respect-internals` option
  - add supporting `InternalsAndPublicCciFilter`
  - see discussions in #4488
  - nits:
    - reorder `GetFilter(...)` conditions for clarity and to reflect precedence
    - use new-ish named parameter syntax
    - sort `using`s
    - add a few more curly braces and newlines

- Avoid GenAPI NRE when naming value types
  - #4488 problem (1)
  - not all tuples have named parameters
  - nit: remove and sort `using`s

- Base `ExcludeAttributesFilter` on `IncludeAllFilter`
  - avoid unintentionally limiting inclusions to `public` members

- Correct generation of faked `[MethodImpl]` attributes
  - avoid C# compilation errors
  - from `[....MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining, NoOptimization)]`
  - to `[....MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining|System.Runtime.CompilerServices.MethodImplOptions.NoOptimization)]`

- Add GenAPI `--exclude-compiler-generated` option
  - #4488 problem (2)
  - add supporting `ExcludeCompilerGeneratedCciFilter`

- Write correct C# for `static` constructors
  - #4488 problem (3)
  - correct method name
  - remove illegal return type and access modifier

- Write correct C# for `unsafe` methods
  - #4488 problem (4)
  - avoid C# compilation errors
  - generate `unsafe` keyword when base constructor call is unsafe
  - generate `unsafe` keyword for unsafe methods in interfaces

- Respect `ICciFilter` when generating fields in value types
  - #2031 aka #2033 aka #4488 problem (6)
  - remove `IsVisibleOutsideAssembly()` use; was at best redundant

- Take `ExcludeAttributesFilter` breaking change

- !fixup! Don't (uselessly) check for attributes on a namespace

- Split and extend `TypeExtensions`
  - split member-related methods into `MemberExtensions`
  - add `MemberExtensions.IsVisibleToFriendAssemblies(...)`
  - add `TypeExtensions.IsConstructorVisibleToFriendAssemblies(...)`

- Use new extension methods in `InternalAndPublicCciFilter`

- Remove extra checks in `ICciFilter` implementations
  - remove parameter `null` checks
  - remove `ContainingTypeDefinition` visibility checks from `Include(ITypeDefinitionMember)`

- Change `ExcludeAttributesFilter` to implement `ICciFilter` directly

- Use a `const` for `[InternalsVisibleTo]` type name

- Add ApiCompat `--respect-internals` option
  - nit: remove and sort `using`s

- Create `private` constructors in `WritePrivateConstructor(...)`

- Add `$(AdditionalApiCompatOptions)` extension point for ApiCompat
  - correct `$(MatchingRefApiCompatBaseline)` setting; read and write same file
  - nits:
    - correct spelling of `$(ImplementationAssemblyAsContract)` property
    - remove incorrect "must be last option" comments

- Do not compare everything in `ICustomModifier` implementations
  - lack of comparer led to "X != X" messages due to varying `InternedKey` and similar properties

- Smarten visibility choice for constructor `WritePrivateConstructor(...) generates
  - choose `private` if internal constructors are likely to be generated by default
  - flatten `IntersectionFilter.Filters` list to make `WritePrivateConstructor(...)` use reasonable
    - also improves overall efficiency

- Add `--exclude-compiler-generated` option to ApiCompat
  - aligns with generation exclusions in GenAPI (if enabled)

- Correct ApiCompat `--contract-depends` command-line argument in the `RunMatchingRefApiCompat` target

- Filter attributes in both directions
  - otherwise, `ValidateApiCompatForSrc` target succeeds while `RunMatchingRefApiCompat` fails in some cases
    - or visa versa

- Simplify a visibility check
  - remove redundant bits

- Remove `$(IntermediateOutputPath)` use in ApiCompat commands
@dougbu
Copy link
Member

dougbu commented Apr 2, 2020

Fixed in d6dd7a0

@dougbu dougbu closed this as completed Apr 2, 2020
@ericstj ericstj added the area-Infrastructure-libraries Area maintained by .NET libraries team: APICompat, AsmDiff, GenAPI, GenFacades, PkgProj, etc label Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Infrastructure-libraries Area maintained by .NET libraries team: APICompat, AsmDiff, GenAPI, GenFacades, PkgProj, etc
Projects
None yet
Development

No branches or pull requests

3 participants