Harden GenAPI #4585

dougbu · 2020-01-03T19:51:42Z

GetAPI generates invalid code when interface property is implemented in a base class #2066
Should GenAPI be removing the StructLayout attribute? #3144 (kind of)
GenAPI issues generate invalid C# for internal types #4488 except problem (5)
- requires significant rewrite to look ahead or maintain sufficient context for #nullable enable gen
- work done overlaps GenApi shouldn't add _dummy and _dummyObject if there are public value and ref fields. #2031 and Avoid generating dummy fields for structs that already contain appropriate public fields #2033
basically, iterate enough to avoid C# compilation errors in aspnet/AspNetCore repo when using --all

(probably easiest to review individual commits)

dougbu · 2020-01-03T19:52:00Z

/fyi @pakrym @JunTaoLuo

src/Microsoft.Cci.Extensions/Extensions/TypeExtensions.cs

src/Microsoft.Cci.Extensions/Filters/ExcludeAttributesFilter.cs

src/Microsoft.DotNet.GenAPI/Program.cs

src/Microsoft.Cci.Extensions/Filters/ExcludeCompilerGeneratedCciFilter.cs

dougbu · 2020-01-07T06:16:50Z

ping @safern

@ericstj @jaredpar is this ready to go?

ericstj · 2020-01-07T17:32:31Z

I'll take another full look.

@dougbu can you make sure you test this package in dotnet/runtime repo before committing? We need to test 1) library build, 2) /t:GenerateReferenceSource from a couple projects: I'd suggest System.Runtime, System.Text.Json, and a few others. @safern do you think you can help provide some suggestions for testing?

jaredpar · 2020-01-07T17:33:56Z

@dougbu

The impact of the change on reference assemblies looks good to me and lines up with what I'd expect in a reference assembly. I'm not very familiar with this project though hence I'm hesitant to "sign off" on the totality of the changes.

src/Microsoft.Cci.Extensions/Extensions/TypeExtensions.cs

safern · 2020-01-07T18:45:39Z

@safern do you think you can help provide some suggestions for testing?

Those assemblies would be good tests.

Also, after those changes running APICompat on those projects if there are significant changes we need to take on those reference assemblies.

When I did the nullable changes, I also had a dummy assembly with a lot of edge cases and ran it against that.

src/Microsoft.Cci.Extensions/Filters/ExcludeCompilerGeneratedCciFilter.cs

src/Microsoft.Cci.Extensions/Filters/ExcludeAttributesFilter.cs

src/Microsoft.Cci.Extensions/Filters/InternalsAndPublicCciFilter.cs

ericstj · 2020-01-08T17:00:04Z

src/Microsoft.Cci.Extensions/Writers/CSharp/CSDeclarationWriter.Methods.cs

-                if (!method.IsExplicitInterfaceMethod()) WriteVisibility(method.Visibility);
+                if (method.IsMethodUnsafe())
+                {
+                    WriteKeyword("unsafe");


It looks to me like WriteInterfaceMethodModifiers is supposed to write modifiers that are common to both methods and interfaces methods. Can you instead just move unsafe check out of WriteMethodModifiers and into WriteInterfaceMethodModifiers? Granted this would change the ordering of the modifiers.

Unfortunately, can't do this because WriteMethodModifiers(...) is also called to generate the modifiers for events and properties. Both can be declared unsafe.

src/Microsoft.DotNet.GenAPI/Program.cs

dougbu

Few questions for you @ericstj…

Otherwise, I'm working on your suggestions. Will also retest AspNetCore and the runtime projects you and @safern pointed me to. (My first reason to clone dotnet/runtime.)

src/Microsoft.Cci.Extensions/Writers/CSharp/CSDeclarationWriter.Methods.cs

src/Microsoft.Cci.Extensions/Filters/ExcludeAttributesFilter.cs

src/Microsoft.Cci.Extensions/Filters/ExcludeCompilerGeneratedCciFilter.cs

src/Microsoft.Cci.Extensions/Filters/InternalsAndPublicCciFilter.cs

src/Microsoft.DotNet.GenAPI/Program.cs

dougbu · 2020-02-17T19:18:42Z

FYI I'm nearly ready to update this PR to resolve the conflict, address review comments, and fix a couple of problems hit in dotnet/aspnetcore. That repo (with changes to use --respect-internals and so on) now generates clean ref/ code and passes ApiCompat checks in both directions with no errors. I'm testing in dotnet/runtime now…

When I did the nullable changes, I also had a dummy assembly with a lot of edge cases and ran it against that.

This was a good idea but not something I did in this case. The dotnet/aspnetcore repo contains all of the edge cases I was trying to fix i.e. I wasn't adding support for new C# features. Of course, please let me know of any corner cases I should double-check; hopefully I can find them where I've already tested.

dougbu · 2020-02-18T00:42:28Z

Generating a private constructor when no other constructor exists doesn't always work in dotnet/runtime because some ref/ code calls filtered-out internal constructors. Which of the following do you prefer?

Add a command-line option controlling the "default" generated constructor, switching it between private and internal
Base the choice between private and internal on --respect-internals
Base the choice between private and internal on whether a filtered-out internal constructor exists i.e. the code could check the constructor collection again in WritePrivateConstructor(...) (without the filter)
Add a list of internal constructors to the *.Manual.cs files. This list shouldn't be incredibly long; I hit is inaccessible due to its protection level problems in only a few cases e.g. for the JsonNode() constructor
Add a UnionFilter that prevents exclusion of a few docIDs from generation as well as a corresponding command-line switch then use that in dotnet/runtime e.g. through $(GenAPIAdditionalParameters)

When testing with dotnet msbuild /t:GenerateReferenceSource or msbuild /t:GenerateReferenceSource in the dotnet/runtime repo, GenApi emits errors about resolving 'System.Private.CoreLib` in a few src directories, including src\libraries\System.Runtime\src\ e.g.

  Unable to resolve assembly 'Assembly(Name=System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e)' referenced by 'Assembly(Name=System.Runtime, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a)'.

The above occurs despite having built the libraries subcategory completely before starting. What am I missing?

Separately from the above, I see some differences in the generation for ExpandoObject:

-        bool System.Collections.Generic.IDictionary<string, object?>.TryGetValue(string key, out object? value) { throw null; }
-        System.Collections.Generic.IEnumerator<System.Collections.Generic.KeyValuePair<string, object?>> System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object?>>.GetEnumerator() { throw null; }
+        bool System.Collections.Generic.IDictionary<string, object?>.TryGetValue(string key, out object value) { throw null; }
+        System.Collections.Generic.IEnumerator<System.Collections.Generic.KeyValuePair<string, object>> System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, object?>>.GetEnumerator() { throw null; }

Unlike other cases where the nullability generation has changed, the above code was correct before I ran GenApi. Before I get deep into debugging the change, is this a known issue that you've been working around?

dougbu · 2020-02-18T00:51:51Z

🆙📅 to current state of my branch.

My updated ref/ code for dotnet/runtime is visible at https://github.com/dotnet/runtime/compare/master...dougbu:dougbu/local.GenApi?expand=1

dougbu · 2020-02-18T00:55:34Z

src/Microsoft.Cci.Extensions/Extensions/TypeExtensions.cs

        }

-        public static bool IsConstructorVisible(this ITypeDefinition type)
+        public static bool IsConstructorVisibleToFriendAssemblies(this ITypeDefinition type)


This method is new

dougbu · 2020-02-18T00:56:26Z

src/Microsoft.Cci.Extensions/Extensions/MemberExtensions.cs

+            return MemberHelper.IsVisibleOutsideAssembly(member);
+        }
+
+        public static bool IsVisibleToFriendAssemblies(this ITypeDefinitionMember member)


Unlike most of this file (which was moved from TypeExtensions), this method and the local IsExplicitImplementationVisible(...) are new

dougbu · 2020-02-18T00:59:23Z

src/Microsoft.Cci.Extensions/Writers/CSharp/CSDeclarationWriter.Methods.cs

@@ -339,7 +354,12 @@ private void WritePrivateConstructor(ITypeDefinition type)
                type.IsStatic)
                return;

-            WriteVisibility(TypeMemberVisibility.Assembly);
+            WriteVisibility(TypeMemberVisibility.Private);


This change needs to be optional in some fashion. See overall comments.

I replied. Can you please record the reasoning for the WritePrivateConstructor in a comment when making your change?

dougbu · 2020-02-18T01:00:53Z

src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets

@@ -32,7 +32,7 @@
    <ApiCompatBaseline Condition="!Exists('$(ApiCompatBaseline)')">$(MSBuildProjectDirectory)\ApiCompatBaseline.txt</ApiCompatBaseline>

    <MatchingRefApiCompatBaseline Condition="!Exists('$(MatchingRefApiCompatBaseline)')">$(MSBuildProjectDirectory)\MatchingRefApiCompatBaseline.$(_apiCompatTargetSuffix).txt</MatchingRefApiCompatBaseline>
-    <MatchingRefApiCompatBaseline Condition="'$(BaselineAllMatchingRefApiCompatError)' != 'true' and !Exists('$(MatchingRefApiCompatBaseline)')">$(MSBuildProjectDirectory)\MatchingRefApiCompatBaseline.txt</MatchingRefApiCompatBaseline>
+    <MatchingRefApiCompatBaseline Condition="!Exists('$(MatchingRefApiCompatBaseline)')">$(MSBuildProjectDirectory)\MatchingRefApiCompatBaseline.txt</MatchingRefApiCompatBaseline>


As mentioned in commit comments, the previous Condition meant baselines were read and written from different files by default

I suspect this was done to make the default behavior to write to TargetFramework-specific baseline files, though I am OK with this change.

dougbu · 2020-02-18T01:01:12Z

src/Microsoft.DotNet.ApiCompat/build/Microsoft.DotNet.ApiCompat.targets

@@ -68,8 +68,8 @@
      <ApiCompatArgs Condition="'$(ApiCompatExcludeAttributeList)' != ''">$(ApiCompatArgs) --exclude-attributes "$(ApiCompatExcludeAttributeList)"</ApiCompatArgs>
      <ApiCompatArgs Condition="'$(ApiCompatEnforceOptionalRules)' == 'true'">$(ApiCompatArgs) --enforce-optional-rules</ApiCompatArgs>
      <ApiCompatArgs Condition="'$(BaselineAllAPICompatError)' != 'true' and Exists('$(ApiCompatBaseline)')">$(ApiCompatArgs) --baseline "$(ApiCompatBaseline)"</ApiCompatArgs>
-      <!-- Must be last option. -->


This was a lie

Probably was true before c380971.

dougbu · 2020-02-18T01:02:20Z

src/Microsoft.DotNet.ApiCompat/Program.cs

-                            excludeNonBrowsable.HasValue(), remapFile.Value(), !skipGroupByAssembly.HasValue(), leftOperandValue, rightOperandValue, excludeAttributes.Value());
+                        var includeInternals = respectInternals.HasValue() &&
+                            contractAssemblies.Any(assembly => assembly.Attributes.HasAttributeOfType(
+                                "System.Runtime.CompilerServices.InternalsVisibleToAttribute"));


No const here because the string is used only in this one place. Any objections?

ericstj · 2020-02-21T21:32:10Z

Base the choice between private and internal on --respect-internals

I'm OK with this.

GenApi emits errors about resolving 'System.Private.CoreLib`

Do you see System.Private.CoreLib being passed in to GenAPI? If not, try adding DependsOnTargets="ResolveReferences" here: https://github.com/dotnet/runtime/blob/86a086377bd63cbe678b6cba5498e7ab4fd9b57d/src/libraries/Directory.Build.targets#L117
Not entirely sure why it doesn't have it /cc @safern @Anipik

I see some differences in the generation for ExpandoObject

Yes, we're working around nullable annotations on explicit interface implementations. #4722

pgrawehr · 2020-03-05T20:00:11Z

Could somebody please take a look at the related PR #4649? I think it's an easy but important fix.

ericstj · 2020-03-05T21:21:27Z

@dougbu let me know if you're waiting on anything here. I can take another look if you have specific places for me to check. I was under the impression you were going to make some changes.

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

- remove parameter `null` checks - remove `ContainingTypeDefinition` visibility checks from `Include(ITypeDefinitionMember)`

nit: remove and sort `using`s

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

- lack of comparer led to "X != X" messages due to varying `InternedKey` and similar properties

….) generates - choose `private` if internal constructurs are likely to be generated by default - flatten `IntersectionFilter.Filters` list to make `WritePrivateConstructor(...)` use reasonable - also improves overall efficiency

- aligns with generation exclusions in GenAPI (if enabled)

…RunMatchingRefApiCompat` target

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

- remove redundant bits

dougbu · 2020-04-02T00:32:58Z

FYI the last update included only the $(IntermediateOutputPath) removal and a final rebase. Will merge when the checks complete and I've run local tests in dotnet/aspnetcore and dotnet/runtime.

- example failure in https://dev.azure.com/dnceng/internal/_build/results?buildId=601199 - undo a small part of #4585 (@1faec9a821cf767d349c7ff18c544876be28cf2d - unfortunately likely to be GC'd soon) - restore generation of `[StructLayout]` attributes on most value types

- example failure in https://dev.azure.com/dnceng/internal/_build/results?buildId=601199 - undo a small part of #4585) - restore generation of `[StructLayout]` attributes on most value types - original commit was 1faec9a

- #5361 - pass property value in `--impl-dirs` argument in `ValidateApiCompatForSrc` target - pass property value in `----contract-depends` argument in `RunMatchingRefApiCompat` target - reverts small part of #4585 - specifically, 24d6981

- #5361 - pass property value in `--impl-dirs` argument in `ValidateApiCompatForSrc` target - reverts small part of #4585 - specifically, restore one line changed in 24d6981

dougbu requested review from jaredpar, ahsonkhan, jkotas, ViktorHofer, ericstj, tannergooding, wtgodbe and safern January 3, 2020 19:51

dougbu commented Jan 3, 2020

View reviewed changes

src/Microsoft.Cci.Extensions/Extensions/TypeExtensions.cs Outdated Show resolved Hide resolved

src/Microsoft.Cci.Extensions/Filters/ExcludeAttributesFilter.cs Outdated Show resolved Hide resolved

safern reviewed Jan 6, 2020

View reviewed changes

src/Microsoft.DotNet.GenAPI/Program.cs Show resolved Hide resolved

jaredpar reviewed Jan 6, 2020

View reviewed changes

src/Microsoft.Cci.Extensions/Filters/ExcludeCompilerGeneratedCciFilter.cs Outdated Show resolved Hide resolved

jaredpar reviewed Jan 6, 2020

View reviewed changes

src/Microsoft.Cci.Extensions/Filters/ExcludeCompilerGeneratedCciFilter.cs Show resolved Hide resolved

ericstj reviewed Jan 7, 2020

View reviewed changes

src/Microsoft.Cci.Extensions/Extensions/TypeExtensions.cs Outdated Show resolved Hide resolved

ericstj reviewed Jan 8, 2020

View reviewed changes

dougbu commented Jan 9, 2020

View reviewed changes

dougbu mentioned this pull request Feb 13, 2020

Add ApiCompat run to repo dotnet/aspnetcore#17959

Closed

dougbu force-pushed the dougbu/fixup.genapi.4488 branch from faadc8a to 38fdbce Compare February 18, 2020 00:50

dougbu commented Feb 18, 2020

View reviewed changes

dougbu mentioned this pull request Feb 27, 2020

Add tests of PushToAzureDevOpsArtifacts #3895

Closed

dougbu added 15 commits April 1, 2020 17:30

Split and extend TypeExtensions

1481b40

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

Use new extension methods in InternalAndPublicCciFilter

16a61d2

Remove extra checks in ICciFilter implementations

52f9361

- remove parameter `null` checks - remove `ContainingTypeDefinition` visibility checks from `Include(ITypeDefinitionMember)`

Change ExcludeAttributesFilter to implement ICciFilter directly

0ce0024

Use a const for [InternalsVisibleTo] type name

882b2f0

Create private constructors in WritePrivateConstructor(...)

c8ad74b

Add ApiCompat --respect-internals option

f88d913

nit: remove and sort `using`s

Add $(AdditionalApiCompatOptions) extension point for ApiCompat

f4623bc

- 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

f38f0f2

- lack of comparer led to "X != X" messages due to varying `InternedKey` and similar properties

Add --exclude-compiler-generated option to ApiCompat

9bee0a0

- aligns with generation exclusions in GenAPI (if enabled)

Correct ApiCompat --contract-depends command-line argument in the `…

00275fa

…RunMatchingRefApiCompat` target

Filter attributes in both directions

5eee082

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

Simplify a visibility check

d8096d1

- remove redundant bits

Remove $(IntermediateOutputPath) use in ApiCompat commands

24d6981

dougbu force-pushed the dougbu/fixup.genapi.4488 branch from 3f3138c to 24d6981 Compare April 2, 2020 00:30

ericstj approved these changes Apr 2, 2020

View reviewed changes

dougbu merged commit d6dd7a0 into master Apr 2, 2020

dougbu deleted the dougbu/fixup.genapi.4488 branch April 2, 2020 04:59

dougbu mentioned this pull request Apr 16, 2020

Fix a build break when using new GenApi #5269

Merged

dougbu mentioned this pull request Apr 27, 2020

ApiCompat no longer passes IntermediateOutputPath #5361

Open

2 tasks

dougbu mentioned this pull request Apr 29, 2020

Restore use of $(IntermediateOutputPath) in ApiCompat commands #5373

Merged

dougbu mentioned this pull request May 13, 2020

Allow setting ApiCompatExcludeNonBrowsable in the csproj #5465

Closed

riarenas mentioned this pull request Jan 28, 2022

Microsoft.DotNet.ApiCompat latest version in missing some older features #8410

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Harden GenAPI #4585

Harden GenAPI #4585

dougbu commented Jan 3, 2020

dougbu commented Jan 3, 2020

dougbu commented Jan 7, 2020

ericstj commented Jan 7, 2020 •

edited by dougbu

Loading

jaredpar commented Jan 7, 2020

safern commented Jan 7, 2020

ericstj Jan 8, 2020

dougbu Jan 12, 2020

dougbu left a comment

dougbu commented Feb 17, 2020

dougbu commented Feb 18, 2020

dougbu commented Feb 18, 2020

dougbu Feb 18, 2020

dougbu Feb 18, 2020

dougbu Feb 18, 2020

ericstj Feb 24, 2020

dougbu Feb 18, 2020

ericstj Feb 24, 2020

dougbu Feb 18, 2020

ericstj Feb 24, 2020

dougbu Feb 18, 2020

ericstj commented Feb 21, 2020

pgrawehr commented Mar 5, 2020

ericstj commented Mar 5, 2020

dougbu commented Apr 2, 2020

Harden GenAPI #4585

Harden GenAPI #4585

Conversation

dougbu commented Jan 3, 2020

dougbu commented Jan 3, 2020

dougbu commented Jan 7, 2020

ericstj commented Jan 7, 2020 • edited by dougbu Loading

jaredpar commented Jan 7, 2020

safern commented Jan 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu left a comment

Choose a reason for hiding this comment

dougbu commented Feb 17, 2020

dougbu commented Feb 18, 2020

dougbu commented Feb 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj commented Feb 21, 2020

pgrawehr commented Mar 5, 2020

ericstj commented Mar 5, 2020

dougbu commented Apr 2, 2020

ericstj commented Jan 7, 2020 •

edited by dougbu

Loading