Skip to content

Conversation

@edgarfgp
Copy link
Contributor

@edgarfgp edgarfgp commented Aug 1, 2025

Description

Aiming to unify LetOrUseBang and LetOrUse. I propose we use Synbinding to model and! which removes the need for SynExprAndBang.

Checklist

  • Test cases added
  • Release notes entry updated

@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2025

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/10.0.100.md

@auduchinok
Copy link
Member

@edgarfgp This is really good, thank you!

What could also be done next is removing the Bang versions of the expressions completely and having an additional bool field instead. That would allow us to unify all the parsing recovery between these variants of expressions.

@edgarfgp edgarfgp marked this pull request as ready for review August 1, 2025 16:09
@edgarfgp edgarfgp requested a review from a team as a code owner August 1, 2025 16:09
@edgarfgp edgarfgp requested a review from Copilot August 1, 2025 16:10
Copy link

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 unifies the representation of and! bindings with regular bindings by replacing the specialized SynExprAndBang type with the general SynBinding type. This change simplifies the AST structure by eliminating duplicate functionality and leveraging the existing binding infrastructure.

Key changes:

  • Replaced SynExprAndBang type with SynBinding for modeling and! clauses
  • Updated all references throughout the codebase to use the unified binding representation
  • Modified test baselines to reflect the new AST structure

Reviewed Changes

Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/Compiler/SyntaxTree/SyntaxTree.fsi Updated interface to use SynBinding list instead of SynExprAndBang list
src/Compiler/SyntaxTree/SyntaxTree.fs Removed SynExprAndBang type definition and updated LetOrUseBang to use SynBinding
src/Compiler/SyntaxTree/SyntaxTrivia.* Removed SynExprAndBangTrivia type as it's no longer needed
src/Compiler/SyntaxTree/ParseHelpers.* Updated mkAndBang to return SynBinding instead of SynExprAndBang
Multiple service files Updated field accessors from SynExprAndBang pattern to SynBinding pattern
Test baseline files Updated expected AST output to reflect new SynBinding structure
Surface area test files Removed SynExprAndBang API surface and added SynBinding.Trivia property
Release notes Added entry documenting the change

@edgarfgp edgarfgp mentioned this pull request Aug 2, 2025
2 tasks
@edgarfgp
Copy link
Contributor Author

edgarfgp commented Aug 4, 2025

This is ready :)

@github-project-automation github-project-automation bot moved this from New to In Progress in F# Compiler and Tooling Aug 4, 2025
@T-Gro T-Gro enabled auto-merge (squash) August 4, 2025 10:00
@T-Gro
Copy link
Member

T-Gro commented Aug 4, 2025

/run test-baseline SurfaceAreaTest

@dotnet dotnet deleted a comment from github-actions bot Aug 4, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2025

🔧 CLI Command Report

  • Command: /run test-baseline
  • Outcome: success

✅ Command succeeded, no changes needed.

edgarfgp and others added 2 commits August 4, 2025 12:45
# Conflicts:
#	tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.SurfaceArea.netstandard20.debug.bsl
#	tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.SurfaceArea.netstandard20.release.bsl
auto-merge was automatically disabled August 4, 2025 10:55

Head branch was pushed to by a user without write access

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Aug 4, 2025

/run test-baseline SurfaceAreaTest

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2025

🔧 CLI Command Report

  • Command: /run test-baseline
  • Outcome: success

✅ Patch applied:
- Files changed:
- Lines changed:

@edgarfgp edgarfgp closed this Aug 4, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in F# Compiler and Tooling Aug 4, 2025
@edgarfgp edgarfgp reopened this Aug 4, 2025
@T-Gro T-Gro merged commit 8680acd into dotnet:main Aug 4, 2025
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants