Skip to content

JS: Fix semantic merge conflict: Implement new signature members in StepInputSig #18513

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

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Jan 16, 2025

Fixes a semantic merge conflict between #18467 and #18298

@github-actions github-actions bot added the JS label Jan 16, 2025
@asgerf asgerf marked this pull request as ready for review January 16, 2025 12:47
@Copilot Copilot AI review requested due to automatic review settings January 16, 2025 12:47
@asgerf asgerf requested a review from a team as a code owner January 16, 2025 12:47
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.

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)
  • javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSummaryPrivate.qll: Language not supported

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

@asgerf asgerf added the no-change-note-required This PR does not need a change note label Jan 16, 2025
@@ -16,6 +16,14 @@ private import semmle.javascript.internal.flow_summaries.ExceptionFlow
*/
class SummarizedCallableBase = string;

class SourceBase extends Unit {
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, you can use the Void type from codeql.util.Void.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do next time. In this case I'd rather unbreak main asap.

@redsun82
Copy link
Contributor

@asgerf wdyt, can we merge this as is to unblock main, and do a follow-up PR maybe implementing @hvitved's suggestion and adding the missing doc comments?

@redsun82
Copy link
Contributor

@asgerf wdyt, can we merge this as is to unblock main, and do a follow-up PR maybe implementing @hvitved's suggestion and adding the missing doc comments?

ah, no the red check is due to the very compilation error this sets out to fix 👍

@redsun82 redsun82 merged commit 133e269 into github:main Jan 16, 2025
11 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants