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

C#: Include type parameters in MaD format for generics #14662

Merged
merged 10 commits into from
Nov 9, 2023

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Nov 2, 2023

This PR changes the MaD format used for generics, to include the names of the type parameters, in both generic types and generic methods. For example, instead of writing

extensions:
  - addsTo:
      pack: codeql/csharp-all
      extensible: summaryModel
      data:
        - ["System.Collections.Generic", "IList<>", True, "Insert", "(System.Int32,T)", "", "Argument[1]", "Argument[this].Element", "value", "manual"]
        - ["System.Linq", "Enumerable", False, "Select<,>", "(System.Collections.Generic.IEnumerable<TSource>,System.Func<TSource,System.Int32,TResult>)", "", "Argument[0].Element", "Argument[1].Parameter[0]", "value", "manual"]

we now instead write

extensions:
  - addsTo:
      pack: codeql/csharp-all
      extensible: summaryModel
      data:
        - ["System.Collections.Generic", "IList<T>", True, "Insert", "(System.Int32,T)", "", "Argument[1]", "Argument[this].Element", "value", "manual"]
        - ["System.Linq", "Enumerable", False, "Select<TSource,TResult>", "(System.Collections.Generic.IEnumerable<TSource>,System.Func<TSource,System.Int32,TResult>)", "", "Argument[0].Element", "Argument[1].Parameter[0]", "value", "manual"]

This aligns with the official documentation, and also makes it clear what type parameters in signatures refer to.

The PR is best reviewed commit-by-commit:

  • Commit 1: Change how we print existing MaD rows to include type parameters.
  • Commit 2: Update the model conversion queries to output existing MaD rows in the new format.
  • Commit 3: Convert existing models using model converter.
  • Commit 4: Change how we parse MaD rows to include type parameters.
  • Commit 5: Manually update some MaD rows.
  • Commit 6: Update model generator to output one file per namespace.
  • Commit 7: Regenerate dotnet/runtime models using new format.
  • Commit 8: Allow for explicit interface names in MaD consistency check.
  • Commit 9: Change note.

@github-actions github-actions bot added the C# label Nov 2, 2023
Copy link
Contributor

github-actions bot commented Nov 2, 2023

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

csharp

Generated file changes for csharp

  • Changes to framework-coverage-csharp.rst:
-    System,"``System.*``, ``System``",25,12149,65,7
+    System,"``System.*``, ``System``",25,11891,67,9
-    Others,"``Dapper``, ``JsonToItemsTaskFactory``, ``Microsoft.ApplicationBlocks.Data``, ``Microsoft.CSharp``, ``Microsoft.EntityFrameworkCore``, ``Microsoft.Extensions.Caching.Distributed``, ``Microsoft.Extensions.Caching.Memory``, ``Microsoft.Extensions.Configuration``, ``Microsoft.Extensions.DependencyInjection``, ``Microsoft.Extensions.DependencyModel``, ``Microsoft.Extensions.FileProviders``, ``Microsoft.Extensions.FileSystemGlobbing``, ``Microsoft.Extensions.Hosting``, ``Microsoft.Extensions.Http``, ``Microsoft.Extensions.Logging``, ``Microsoft.Extensions.Options``, ``Microsoft.Extensions.Primitives``, ``Microsoft.Interop``, ``Microsoft.NET.Build.Tasks``, ``Microsoft.NETCore.Platforms.BuildTasks``, ``Microsoft.VisualBasic``, ``Microsoft.Win32``, ``MySql.Data.MySqlClient``, ``Newtonsoft.Json``, ``Windows.Security.Cryptography.Core``",,568,138,
+    Others,"``Dapper``, ``ILCompiler``, ``Internal.IL``, ``Internal.Pgo``, ``Internal.TypeSystem``, ``JsonToItemsTaskFactory``, ``Microsoft.ApplicationBlocks.Data``, ``Microsoft.CSharp``, ``Microsoft.Diagnostics.Tools.Pgo``, ``Microsoft.EntityFrameworkCore``, ``Microsoft.Extensions.Caching.Distributed``, ``Microsoft.Extensions.Caching.Memory``, ``Microsoft.Extensions.Configuration``, ``Microsoft.Extensions.DependencyInjection``, ``Microsoft.Extensions.DependencyModel``, ``Microsoft.Extensions.FileProviders``, ``Microsoft.Extensions.FileSystemGlobbing``, ``Microsoft.Extensions.Hosting``, ``Microsoft.Extensions.Http``, ``Microsoft.Extensions.Logging``, ``Microsoft.Extensions.Options``, ``Microsoft.Extensions.Primitives``, ``Microsoft.Interop``, ``Microsoft.NET.Build.Tasks``, ``Microsoft.NETCore.Platforms.BuildTasks``, ``Microsoft.VisualBasic``, ``Microsoft.Win32.SafeHandles``, ``MySql.Data.MySqlClient``, ``Newtonsoft.Json``, ``Windows.Security.Cryptography.Core``",,1111,138,
-    Totals,,25,12724,397,7
+    Totals,,25,13009,399,9
  • Changes to framework-coverage-csharp.csv:
+ ILCompiler,,,80,,,,,,,,,,,,,,80,
+ Internal.IL,,,68,,,,,,,,,,,,,,66,2
+ Internal.Pgo,,,9,,,,,,,,,,,,,,8,1
+ Internal.TypeSystem,,,352,,,,,,,,,,,,,,316,36
+ Microsoft.Diagnostics.Tools.Pgo,,,12,,,,,,,,,,,,,,12,
- Microsoft.Extensions.Caching.Memory,,,46,,,,,,,,,,,,,,45,1
+ Microsoft.Extensions.Caching.Memory,,,38,,,,,,,,,,,,,,37,1
- Microsoft.Extensions.Configuration,,,83,,,,,,,,,,,,,,80,3
+ Microsoft.Extensions.Configuration,,,79,,,,,,,,,,,,,,76,3
- Microsoft.Extensions.DependencyInjection,,,62,,,,,,,,,,,,,,62,
+ Microsoft.Extensions.DependencyInjection,,,60,,,,,,,,,,,,,,60,
- Microsoft.Extensions.FileProviders,,,16,,,,,,,,,,,,,,16,
+ Microsoft.Extensions.FileProviders,,,17,,,,,,,,,,,,,,17,
- Microsoft.Extensions.FileSystemGlobbing,,,15,,,,,,,,,,,,,,13,2
+ Microsoft.Extensions.FileSystemGlobbing,,,16,,,,,,,,,,,,,,14,2
- Microsoft.Extensions.Hosting,,,17,,,,,,,,,,,,,,16,1
+ Microsoft.Extensions.Hosting,,,20,,,,,,,,,,,,,,19,1
- Microsoft.Extensions.Logging,,,37,,,,,,,,,,,,,,37,
+ Microsoft.Extensions.Logging,,,39,,,,,,,,,,,,,,39,
- Microsoft.Interop,,,27,,,,,,,,,,,,,,27,
+ Microsoft.Interop,,,60,,,,,,,,,,,,,,60,
- Microsoft.Win32,,,8,,,,,,,,,,,,,,8,
+ Microsoft.Win32.SafeHandles,,,4,,,,,,,,,,,,,,4,
- System,65,25,12149,,8,8,9,,,4,3,33,1,17,3,4,10163,1986
+ System,67,25,11891,,8,8,9,,,4,5,33,1,17,3,4,9906,1985

@hvitved hvitved force-pushed the csharp/mad-generics branch 3 times, most recently from 510a38a to de1f2c8 Compare November 7, 2023 14:11
@hvitved hvitved changed the title C#: Use different MaD format for generics C#: Include type parameters in MaD format for generics Nov 7, 2023
@hvitved hvitved force-pushed the csharp/mad-generics branch 4 times, most recently from fcf2cb1 to e33e414 Compare November 8, 2023 12:44
@hvitved hvitved force-pushed the csharp/mad-generics branch from e33e414 to 5ae025f Compare November 9, 2023 07:46
@hvitved hvitved marked this pull request as ready for review November 9, 2023 07:46
@hvitved hvitved requested a review from a team as a code owner November 9, 2023 07:46
@michaelnebel michaelnebel self-requested a review November 9, 2023 09:09
extensions:
{0}"""
for entry in extensions:
target = os.path.join(self.generatedFrameworks, entry + extension)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you also try running this for java?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are interested then you can run modify the mad_modelDiff.yml workflow file to also be triggered on the shared python code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to run that on my PR branch, do you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you just change the paths section of the workflow file? If that doesn't work - then I don't know.

@michaelnebel
Copy link
Contributor

  • Do you know which summaries in System that disappeared?
  • Will you start a DCA run?

@@ -66,8 +66,8 @@ extensions:
- ["System.Collections.Generic", "SortedDictionary<TKey,TValue>", False, "get_Values", "()", "", "Argument[this].Element.Property[System.Collections.Generic.KeyValuePair<,>.Value]", "ReturnValue.Element", "value", "manual"]
- ["System.Collections.Generic", "SortedDictionary<TKey,TValue>+KeyCollection", False, "GetEnumerator", "()", "", "Argument[this].Element", "ReturnValue.Property[System.Collections.Generic.SortedDictionary<,>+KeyCollection+Enumerator.Current]", "value", "manual"]
- ["System.Collections.Generic", "SortedDictionary<TKey,TValue>+ValueCollection", False, "GetEnumerator", "()", "", "Argument[this].Element", "ReturnValue.Property[System.Collections.Generic.SortedDictionary<,>+ValueCollection+Enumerator.Current]", "value", "manual"]
- ["System.Collections.Generic", "SortedList<,>+KeyList", False, "Clear", "()", "", "Argument[this].WithoutElement", "Argument[this]", "value", "manual"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you searched all the yaml files using the regex <,*>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes; the only remaining occurrences should be those inside a Property or Field token, e.g.

["System.Collections.Generic", "Dictionary<TKey,TValue>", False, "Add", "(System.Collections.Generic.KeyValuePair<TKey,TValue>)", "", "Argument[0].Property[System.Collections.Generic.KeyValuePair<,>.Key]", "Argument[this].Element.Property[System.Collections.Generic.KeyValuePair<,>.Key]", "value", "manual"]

This will change to

["System.Collections.Generic", "Dictionary<TKey,TValue>", False, "Add", "(System.Collections.Generic.KeyValuePair<TKey,TValue>)", "", "Argument[0].Property[System.Collections.Generic.KeyValuePair`2.Key]", "Argument[this].Element.Property[System.Collections.Generic.KeyValuePair`2.Key]", "value", "manual"]

once #14589 lands.

@hvitved
Copy link
Contributor Author

hvitved commented Nov 9, 2023

  • Do you know which summaries in System that disappeared?

That would be the auto-generated summaries from 136adb2.

  • Will you start a DCA run?

Already did :-)

Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for doing this! 👍

@hvitved hvitved merged commit 94d08aa into github:main Nov 9, 2023
17 checks passed
@hvitved hvitved deleted the csharp/mad-generics branch November 9, 2023 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants