Skip to content
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

MakeInvocationExpression helper doesn't belong in SyntheticBoundNodeFactory #74275

Closed
AlekseyTs opened this issue Jul 5, 2024 · 0 comments
Closed
Assignees
Labels
Area-Compilers Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Milestone

Comments

@AlekseyTs
Copy link
Contributor

Multiple helpers in SyntheticBoundNodeFactory make use of MakeInvocationExpression helper,
which actually uses a Binder to bind an invocation expression (do overload resolution, apply necessary
conversions to the arguments, etc.). Therefore, there is no guarantee that it is going to produce a node in its lowered form, which we generally expect from SyntheticBoundNodeFactory to do. The helper is used indirectly in lowering and in the post lowering phases of the compiler. When the operation is performed against an unknown set of overloads (and that is how it is used today) the result is unpredictable in terms of what we might get back. For example, we might get back a node that is not going to pass DiagnosticPass checks with respect to operations allowed in an expression tree, resulting in a compiler crash (see, for example, #74163 caused by exactly that). Or we might get back a not lowered node in a post lowering phase, etc.

I propose the following action:

  1. Remove the helper in the current form from SyntheticBoundNodeFactory
  2. If a binding against an unknown set of overload should occur, that should be done in initial binding phase, the resulting node should be stored "on a side", used by the LocalRewriter and be a subject to DiagnosticPass checks. We have plenty examples of how to do that, just search for use of "placeholders" in bound nodes.
  3. Otherwise, we should consider binding against a known and restricted set of overloads with known argument types, so that we can guarantee that we are going to get back a node that doesn't require lowering and cannot contain "forbidden" operations.
        internal BoundExpression MakeInvocationExpression(
            BinderFlags flags,
            SyntaxNode node,
            BoundExpression receiver,
            string methodName,
            ImmutableArray<BoundExpression> args,
            BindingDiagnosticBag diagnostics,
            ImmutableArray<TypeSymbol> typeArgs = default(ImmutableArray<TypeSymbol>),
            bool ignoreNormalFormIfHasValidParamsParameter = false)
        {
            if (_binder is null || _binder.Flags != flags)
            {
                _binder = new SyntheticBinderImpl(this).WithFlags(flags);
            }

            return _binder.MakeInvocationExpression(
                node,
                receiver,
                methodName,
                args,
                diagnostics,
                typeArgs: typeArgs.IsDefault ? default(ImmutableArray<TypeWithAnnotations>) : typeArgs.SelectAsArray(t => TypeWithAnnotations.Create(t)),
                allowFieldsAndProperties: false,
                ignoreNormalFormIfHasValidParamsParameter: ignoreNormalFormIfHasValidParamsParameter);
        }
@AlekseyTs AlekseyTs added Area-Compilers Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality labels Jul 5, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Jul 5, 2024
@jaredpar jaredpar added this to the Backlog milestone Jul 10, 2024
@AlekseyTs AlekseyTs self-assigned this Oct 1, 2024
AlekseyTs added a commit to AlekseyTs/roslyn that referenced this issue Oct 2, 2024
AlekseyTs added a commit to AlekseyTs/roslyn that referenced this issue Oct 8, 2024
AlekseyTs added a commit to AlekseyTs/roslyn that referenced this issue Oct 9, 2024
AlekseyTs added a commit to AlekseyTs/roslyn that referenced this issue Oct 11, 2024
@AlekseyTs AlekseyTs added Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented and removed 3 - Working untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Projects
None yet
Development

No branches or pull requests

2 participants