Avoid generating dummy fields for structs that already contain appropriate public fields #2033
Conversation
…riate public fields
|
@jaredpar - who do you think the right person to review this is? (not sure @weshaggard cares anymore...grin) |
|
Unfortunately everyone who understood this has left the various teams. At least that I'm aware of. @weshaggard may know someone who is still here that can review. My 2 cents though: the current state isn't broken, it's just verbose. There are no tests for this code and it's going to take effort to find a reviewer. I'd leave as is for now. |
|
How does it block ASP.NET adoption? That's not specified anywhere. |
|
We have a type that has |
|
Then this doesn't seem like the right fix. It will still leave the dummy field for other cases which need a dummy field and they'll hit the same error. Shouldn't we be fixing that? |
We should definitely fix that too. But my goal here is to unblock ASP.NET Core 3.0 targeting pack work. This PR is enough to accomplish that. |
|
Can we get some traction here, please? This is the last thing that's blocking targeting pack work on our side. cc @muratg |
|
my 2 cents:
|
|
The changes look reasonable to me but I shouldn't be the one signing off on the changes given that I won't be the one fixing issues if they occur. I'd suggest one of @ahsonkhan @terrajobst @ericstj @karelz as potential reviewers. @jaredpar from a quick look it appears this is a valid fix and if we don't need to include a dummy field we shouldn't, so while #2031 might not be 100% necessary it is also not bad to take a fix for it. I also agree that #2030 should be fixed as well, however even if that was fixed I'm not sure how we would get a struct layout that matches what the implementation contains while we add a new dummy field. @pakrym you can also unblock yourself by excluding this type from genapi and providing it manually until the tooling can be fixed. |
|
It's been almost a month. Do you think this is still needed @pakrym ? |
|
We unblocked ourselves by manually editing the generated files. It would be nice to get this in any way to avoid future problems and additional work to maintain manually edited refs. |
|
I think the correct fix is to entirely omit dummy fields if the type has an explicit layout and instead expose all fields from the implementation as-is (preserving modifiers obviously). The reason being that explicit layout basically means there isn't much room for implementation details anyways -- they are part of your API contract. |
terrajobst
requested review from
ericstj,
jkotas and
stephentoub
and removed request for
weshaggard
Sounds about right. I'd feel much more comfortable about selectively exposing things already defined in impl rather a field called dummy. What does csc /refout do? |
|
@terrajobst, can you help me to understand that better? This may be academic, because at least for coreclr/corefx, looking through all of our LayoutKind.Explicit structs, they fall into one of two categories:
That said, for a decent number of the internal structs that are Explicit, the layout actually is just an implementation detail. For example: |
I would recommend doing this. The proposed fix fixes some cases, but breaks other cases: If you have a struct with both private and public fields, the fix may make the C# compiler to think that that the struct is fully initialized when it actually is not. I am not sure whether it is worth inventing the correct fix for this corner case. It should be only ever a problem for APIs that are violating API design guidelines. |
|
@pakrym - does it make sense to close this PR? |
The only relevant case is (2) here (because we're talking about GenAPI). My statement was meant to mean "if you expose a struct with explicit layout in your API, you'e surrendering your right to change the layout of private fields, thus GenAPI should encode the type as is from the implementation".
That's fair, but I'm not sure it's worth optimizing for this case. My proposal isn't meant to make GenAPI work for arbitrary design goals; but I think the heuristic is good enough. |
|
I'll leave the decision here to @anurse |
|
Since we've worked around this using manual generation, I'm ok to close this. |
…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)
- 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
Fixes: #2031