Should GenAPI be removing the StructLayout attribute? #3144
Comments
|
This is another case where I think analyzers looking for these are not uncommon. |
|
This is also a case where it can impact interop scenarios and where it is already defined alongside the struct (rather than being a UserDefined attribute) today. For example, |
|
Does adding this to the reference assemblies limit the types of changes we can make in the future that previously would have been considered compatible? If no, then it seems reasonable to add back. |
|
In wast majority of cases, the struct layout is internal implementation detail and it does not make sense to include it in reference assemblies. In the very rare cases where the struct layout is really part of the public contract (e.g. the struct has public overlapping fields with explicit offsets), I think it is fine to expose the struct layout in reference assembly. However, I do not think it is necessary to make GenAPI to handle this case given how rare it is in the framework and that is also close to impossible to handle all corner cases automatically. I think it is fine to depend on manual description for cases like these. #2033 is an example where this situation occurred. |
Do you have specific real-world examples for these? |
I'm unsure of first party analyzers, but I've written an analyzer to help catch cases like using structs marked I don't think that we generally include |
Question: Where would the difficulty come from here? Could we just retain the attribute everywhere it exists in the ref today, or add it if it exists in the source? |
|
The question is really whether exact struct layout is part of the public contract or not. The exact struct layout is not part of the public contract today for 99+% of the structs in the public surface. Thus it does not make sense to include any of the implementation details related to struct layout in the reference assemblies: StructLayoutAttribute, all fields with exact types, etc. If we changed our mind and decided that we do want to make struct layout part of the public contract, we would want to include all information related to the struct layout: StructLayoutAttribute, all fields with exact types, etc. It does not make sense to include partial information about the struct layout. |
It might be worth mentioning that ECMA-335 (the runtime spec) has an attached TR-84, which defines the BCL types and their signatures. They all include the TR-84 does not, however, specify what non-public fields exist. |
|
We can fix that in the spec once we get to updating it to make it more sensible. |
- see #3144 discussions (not an exact fix for that request) - `LayoutKind.Auto` is _not_ the default for value types nit: sort `using`s
- see #3144 discussions (not an exact fix for that request) - `LayoutKind.Auto` is _not_ the default for value types nit: sort `using`s
- see #3144 discussions (not an exact fix for that request) - `LayoutKind.Auto` is _not_ the default for value types nit: sort `using`s
- see #3144 discussions (not an exact fix for that request) - `LayoutKind.Auto` is _not_ the default for value types nit: sort `using`s
- see #3144 discussions (not an exact fix for that request) - `LayoutKind.Auto` is _not_ the default for value types nit: sort `using`s
- 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
|
Somewhat improved in d6dd7a0 |
ericstj
added
the
area-Infrastructure-libraries
dotnet/corefx#35557 (comment)
If we are excluding this attribute, we should excluded
FieldOffsetAttributeas well.https://github.com/dotnet/corefx/blob/26b932e3c56702382d13ece010b300d3181d2891/eng/DefaultGenApiDocIds.txt#L31
Otherwise, we should keep this attribute.
The text was updated successfully, but these errors were encountered: