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

GenApi shouldn't add _dummy and _dummyObject if there are public value and ref fields. #2031

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

Comments

@pakrym
Copy link
Contributor

pakrym commented Feb 12, 2019

For example in the following struct object _dummy is not required because RenderTreeFrame has public ref fields which should be sufficient for compiler checks.

    [System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Explicit)]
    public readonly partial struct RenderTreeFrame
    {
        private readonly object _dummy;
        [System.Runtime.InteropServices.FieldOffsetAttribute(8)]
        public readonly int AttributeEventHandlerId;
        [System.Runtime.InteropServices.FieldOffsetAttribute(16)]
        public readonly string AttributeName;
        [System.Runtime.InteropServices.FieldOffsetAttribute(24)]
        public readonly object AttributeValue;
        [System.Runtime.InteropServices.FieldOffsetAttribute(12)]
        public readonly int ComponentId;
        [System.Runtime.InteropServices.FieldOffsetAttribute(16)]
        public readonly System.Action<object> ComponentReferenceCaptureAction;
        [System.Runtime.InteropServices.FieldOffsetAttribute(8)]
        public readonly int ComponentReferenceCaptureParentFrameIndex;
}

cc @jaredpar am I correct in this assumption?

@rynowak
Copy link
Member

rynowak commented Aug 28, 2019

Updoot for this. Having to maintain a manual ref-assembly type just resulted in an ask mode bug for us in 3.0 - https://github.com/aspnet/AspNetCore/pull/13522/files

This is risky business for us because once you go manual with a ref-assembly type, you lose all validation.

dougbu added a commit that referenced this issue Jan 3, 2020
- #2031 aka #2033 aka #4488 problem (6)
- remove `IsVisibleOutsideAssembly()` use; was at best redundant
@dougbu dougbu mentioned this issue Jan 3, 2020
dougbu added a commit to dotnet/aspnetcore that referenced this issue Feb 4, 2020
…I commands

- exclude `@(Reference)` items for .Sources projects
- ignore CS0169 warnings in ref/ projects
  - some newly-generated types encounter this warning
- add exclusions related to nullable reference types due to dotnet/arcade#4488 (part 5)
- add exclusions to avoid System.Buffers conflicts with MessagePack submodule
- add exclusion for async use
- remove exclusions for properties w/ `internal set`
- remove exclusions for GenAPI NREs
- remove exclusions related to dotnet/arcade#2031 aka dotnet/arcade#2033 aka dotnet/arcade#4488 (part 6)
dougbu added a commit that referenced this issue Feb 18, 2020
- #2031 aka #2033 aka #4488 problem (6)
- remove `IsVisibleOutsideAssembly()` use; was at best redundant
dougbu added a commit that referenced this issue Mar 24, 2020
- #2031 aka #2033 aka #4488 problem (6)
- remove `IsVisibleOutsideAssembly()` use; was at best redundant
dougbu added a commit that referenced this issue Mar 31, 2020
- #2031 aka #2033 aka #4488 problem (6)
- remove `IsVisibleOutsideAssembly()` use; was at best redundant
dougbu added a commit that referenced this issue Apr 2, 2020
- #2031 aka #2033 aka #4488 problem (6)
- remove `IsVisibleOutsideAssembly()` use; was at best redundant
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
dougbu added a commit to dougbu/aspnetcore that referenced this issue Oct 2, 2021
…I commands

- exclude `@(Reference)` items for .Sources projects
- generate ref/ C# files only for the default TFM
- ignore CS0169 warnings in ref/ projects
  - some newly-generated types encounter this warning
- add exclusions related to nullable reference types due to dotnet/arcade#4488 (part 5)
- add exclusions to avoid System.Buffers conflicts with MessagePack submodule
- add exclusion for async use
- remove exclusions for properties w/ `internal set`
- remove exclusions for GenAPI NREs
- remove exclusions related to dotnet/arcade#2031 aka dotnet/arcade#2033 aka dotnet/arcade#4488 (part 6)
@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

4 participants