Skip to content

Use canHaveFlowNode in checkIfExpressionRefinesParameter #1145

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

Closed
wants to merge 2 commits into from

Conversation

Copilot
Copy link

@Copilot Copilot AI commented Jun 10, 2025

This PR ports the changes from microsoft/TypeScript#58816 to the Go implementation.

TypeScript Change Summary

The original TypeScript change replaced a type assertion pattern:

const antecedent = (expr as Expression & { flowNode?: FlowNode; }).flowNode ||

with a proper type guard function:

const antecedent = canHaveFlowNode(expr) && expr.flowNode ||

Go Change Summary

In Go, this translates to replacing getFlowNodeOfNode(expr) with an explicit canHaveFlowNode check pattern:

Before:

antecedent := getFlowNodeOfNode(expr)

After:

var antecedent *ast.FlowNode
if canHaveFlowNode(expr) {
    antecedent = expr.FlowNodeData().FlowNode
}

This change improves code clarity by using the explicit type guard function canHaveFlowNode instead of relying on the internal helper getFlowNodeOfNode, making the code more consistent with the TypeScript implementation style.

The semantics remain exactly the same - both approaches check if the node has flow node data and extract it if available, but the new approach is more explicit and matches the TypeScript codebase pattern.

All tests pass and build succeeds

Fixes #1129.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] Port TypeScript PR #58816: Use canHaveFlowNode in checkIfExpressionRefinesParameter Use canHaveFlowNode in checkIfExpressionRefinesParameter Jun 10, 2025
@Copilot Copilot AI requested a review from andrewbranch June 10, 2025 00:44
Copilot finished work on behalf of andrewbranch June 10, 2025 00:44
@Andarist
Copy link
Contributor

I don't think this is needed at all. canHaveFlowNode served a language/engine-specific purpose in Strada and that isn't applicable to Corsa.

@andrewbranch
Copy link
Member

Notably, canHaveFlowNode is defined but never used. The PR has identical semantics to main, but it should have either done nothing or deleted the unused function for extra credit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port TypeScript PR #58816: Use canHaveFlowNode in checkIfExpressionRefinesParameter
3 participants