Skip to content

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 AI 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 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

@github-actions
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
@LWSimpkins LWSimpkins deleted the csharp-update-MaD-Uri-upstream branch May 23, 2025 02:44
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