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

csharp update MaD for System.Uri #19142

Merged
merged 2 commits into from
Mar 31, 2025

Conversation

LWSimpkins
Copy link
Contributor

  • Add missing constructors, or fix where the output field in the summary model was a synthetic field that did not match the rest of the Uri models
  • Fix the out parameter for TryCreate
  • Add missing parameter access, or fix where the input field in the summary model was a synthetic field that did not match the rest of the Uri models

@Copilot Copilot bot review requested due to automatic review settings March 28, 2025 03:50
@LWSimpkins LWSimpkins requested a review from a team as a code owner March 28, 2025 03:50
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the System.Uri model definitions to correctly model the data flows, including fixing the out parameter for TryCreate and adding missing constructor overloads for improved summary consistency.

  • Updated TryCreate overloads to correct the out parameter mapping.
  • Added additional Uri constructors with modified parameter access.
  • Updated change notes to document these model changes.

Reviewed Changes

Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.

File Description
csharp/ql/lib/ext/System.model.yml Updated model entries for System.Uri with new and corrected overloads
csharp/ql/lib/change-notes/2025-03-27.md Added a minor analysis note for the updated System.Uri models
Files not reviewed (2)
  • csharp/ql/test/library-tests/dataflow/library/FlowSummaries.expected: Language not supported
  • csharp/ql/test/library-tests/dataflow/library/FlowSummariesFiltered.expected: Language not supported
Comments suppressed due to low confidence (2)

csharp/ql/lib/ext/System.model.yml:791

  • Please confirm that having two entries for the (System.Uri,System.String) constructor with different parameter mappings (lines 791 and 792) is intentional; if not, consolidate the entries to avoid ambiguity.
["System", "Uri", False, "Uri", "(System.Uri,System.String)", "", "Argument[0]", "Argument[this]", "taint", "manual"]

csharp/ql/lib/ext/System.model.yml:793

  • Please verify that the duplicate definition of the (System.Uri,System.String,System.Boolean) constructor with differing argument indexes (lines 793 and 794) is intended to capture all required parameter access scenarios.
["System", "Uri", False, "Uri", "(System.Uri,System.String,System.Boolean)", "", "Argument[0]", "Argument[this]", "taint", "manual"]

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

Copy link
Contributor

⚠️ 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``",47,12241,54,5
+    System,"``System.*``, ``System``",47,12255,54,5
-    Totals,,107,14520,400,9
+    Totals,,107,14534,400,9
  • Changes to framework-coverage-csharp.csv:
- System,54,47,12241,,6,5,5,,,4,1,,33,2,,6,15,17,4,3,,5941,6300
+ System,54,47,12255,,6,5,5,,,4,1,,33,2,,6,15,17,4,3,,5955,6300

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.

LGTM!
Will trigger a DCA run.

@michaelnebel
Copy link
Contributor

DCA looks good!

@michaelnebel
Copy link
Contributor

Re-triggering the CI as it is stuck for some reason.

@michaelnebel michaelnebel merged commit 1c93e53 into github:main Mar 31, 2025
21 checks passed
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