Add shims to netstandard2.1 for types that used to have OOB packages#1180
Add shims to netstandard2.1 for types that used to have OOB packages#1180wtgodbe merged 8 commits intodotnet:masterfrom
Conversation
| @@ -1,13 +1,25 @@ | |||
| Compat issues with assembly System.Memory: | |||
| CannotRemoveAttribute : Attribute 'Microsoft.Cci.DummyTypeReference' exists on 'System.Memory<T>' in the contract but not the implementation. | |||
There was a problem hiding this comment.
This looks like there are type resolution errors. Are those expected?
|
@ericstj this should be ready for review now, PTAL when you get the chance. Thanks for all the help! |
|
Nice branch name |
| <RestoreOutputPath>$(IntermediateOutputPath)$(TargetFramework)\</RestoreOutputPath> | ||
| <ProjectAssetsFile>$(RestoreOutputPath)/project.assets.json</ProjectAssetsFile> | ||
| <MSBuildProjectExtensionsPath>$(RestoreOutputPath)</MSBuildProjectExtensionsPath> | ||
| <_InitialMSBuildProjectExtensionsPath>$(MSBuildProjectExtensionsPath)</_InitialMSBuildProjectExtensionsPath> |
There was a problem hiding this comment.
Did you still need this hack? Makes me nervous we are using the internal property.
There was a problem hiding this comment.
I'm not sure how else to get around the issue, we do need to get the targets file imported for netstandard.library.
There was a problem hiding this comment.
I think the reason we did this hack was due to order of import / targetframework definition. I believe you could find a different order of defining properties that would get this set eariler (if you move some things into Directory.Build.props). Alternatively you could just ignore the targets and manually include the items. I'd prefer either of those approaches over using the private property (I know we spent some time figuring this out but we shouldn't sink ourselves in the fragile solution because of that).
There was a problem hiding this comment.
I just pushed a commit that resolves the NetStandard.Library assets explicitly & removes this workaround
| Compat issues with assembly System.Xml.ReaderWriter: | ||
| CannotRemoveAttribute : Attribute 'System.ComponentModel.EditorBrowsableAttribute' exists on 'System.Xml.Schema.XmlSchema' in the contract but not the implementation. | ||
| Total Issues: 9 | ||
| Compat issues with assembly System.Memory: |
There was a problem hiding this comment.
I'm guessing these are real errors, which we weren't seeing before since we just added System.Memory to the contract
There was a problem hiding this comment.
Does that mean we broke the API surface System.Memory previously exposed to netstandard2.0 consumers? That'd be a bad issue and should be addressed by adding that surface area to netstandard2.1, or producing a new version of System.Memory that is non breaking. I don't think that's what is happening, but we need to have an understanding and not guess. We should be filing issues if we introduce baselines that imply breaking changes. @terrajobst
There was a problem hiding this comment.
Looks like that's the case, these members (e.g. TryGetMemoryManager) exist in the Ref in CoreFx but aren't defined in the Standard (https://github.com/dotnet/corefx/blob/1f9b84a0804e868c7e0f37a3c10fbaf7c735ae14/src/System.Memory/ref/System.Memory.cs#L471). @terrajobst what's the best way for us to add these in Standard?
There was a problem hiding this comment.
Please open an issue and make sure these are tracked for ns2.1.
| </PackageReference> | ||
| </ItemGroup> | ||
|
|
||
| <Target Name="ResolveNetStandardLibraryAssets" |
ericstj
left a comment
There was a problem hiding this comment.
Thanks for following up on my suggestions.
| Compat issues with assembly netstandard: | ||
| TypeCannotChangeClassification : Type 'System.RuntimeArgumentHandle' is a 'ref struct' in the implementation but is a 'struct' in the contract. | ||
| TypeCannotChangeClassification : Type 'System.TypedReference' is a 'ref struct' in the implementation but is a 'struct' in the contract. | ||
| CannotSealType : Type 'System.ComponentModel.BaseNumberConverter' is effectively (has a private constructor) sealed in the implementation but not sealed in the contract. |
There was a problem hiding this comment.
It may be worth adding functionality to APICompat so that we can produce a baseline file that doesn't have these duplicate entries (in this case, one each for netstandard, system, and System.ComponentModel). We could separate the concept of baseline files (like this one) with dev-friendly baseline files (without the duplicates). APICompat reads the former, and writes both.
Resolves https://github.com/dotnet/corefx/issues/36719 & #1063. These TypeForward files were generated with
GenApi, using thenetstandard2.0assets from the packages I added tonetstandard.depproj.Still to do:
netstandard2.1to theSystem.Reflection.Emit*packages in CoreFx - will be done by Mark System.Reflection.Emit* as inbox on netstandard2.1 corefx#37532@ericstj @terrajobst PTAL
CC @bartonjs