Skip to content

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Nov 28, 2022

As discussed with @aschackmull, it is best to have at most one ParameterNode per ParameterPosition and DataFlowCallable. This PR ensures that each DataFlowCallable has at most one ParameterNode with hash-splat ParameterPosition.

@github-actions github-actions bot added the Ruby label Nov 28, 2022
@hvitved hvitved force-pushed the ruby/unique-hash-splat-param branch from 3bb5c5a to 9453bb8 Compare November 29, 2022 09:01
@hvitved hvitved force-pushed the ruby/unique-hash-splat-param branch from 9453bb8 to c67262a Compare November 29, 2022 09:15
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Nov 29, 2022
@hvitved hvitved marked this pull request as ready for review November 29, 2022 11:17
@hvitved hvitved requested review from a team as code owners November 29, 2022 11:17
MathiasVP
MathiasVP previously approved these changes Nov 29, 2022
@MathiasVP
Copy link
Contributor

Feel free to ignore the Swift CI failures. They should be fixed by merging in the recent main.

michaelnebel
michaelnebel previously approved these changes Nov 29, 2022
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.

C#: Looks plausible to me.

@hvitved hvitved dismissed stale reviews from michaelnebel and MathiasVP via b33f592 November 30, 2022 12:39
@hvitved hvitved force-pushed the ruby/unique-hash-splat-param branch from c67262a to b33f592 Compare November 30, 2022 12:39
@hvitved hvitved requested a review from a team as a code owner November 30, 2022 12:39
@github-actions github-actions bot added the Go label Nov 30, 2022
@hvitved
Copy link
Contributor Author

hvitved commented Nov 30, 2022

Rebased to resolve semantic merge conflict (Go data flow libraries now need to be merged as well).

Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

Go 👍🏻

Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

Go 👍🏻

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.

C# LGTM

@calumgrant calumgrant requested a review from aibaars December 5, 2022 09:26
Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

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

Looks good to me. Would it be possible to add a test case for Ruby?

@hvitved
Copy link
Contributor Author

hvitved commented Dec 5, 2022

Looks good to me. Would it be possible to add a test case for Ruby?

Rather, we will be adding some consistency checks to the shared data flow library.

@hvitved hvitved merged commit faca4b5 into github:main Dec 5, 2022
@hvitved hvitved deleted the ruby/unique-hash-splat-param branch December 5, 2022 10:53
@aibaars
Copy link
Contributor

aibaars commented Dec 5, 2022

Rather, we will be adding some consistency checks to the shared data flow library.

That's even better!

@hvitved
Copy link
Contributor Author

hvitved commented Dec 5, 2022

Consistency checks: #11564

RasmusWL added a commit to RasmusWL/codeql that referenced this pull request Jan 27, 2023
Similar to the Ruby changes from
github#11461

I feel the change to `DataFlowFunciton.getParameter` where we use
`not exists(func.getArgByName(_))` is not very great, but I was not allowed
to use `not exists(this.getParameter(any(ParameterPosition _).isKeyword(_)))`
because of negative recursion.
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.

5 participants