Propagate EffectiveScope to parameters of anonymous delegate types.#63787
Propagate EffectiveScope to parameters of anonymous delegate types.#63787AlekseyTs merged 2 commits intodotnet:mainfrom
Conversation
|
@cston, @RikkiGibson, @dotnet/roslyn-compiler Please review |
| /// <summary> | ||
| /// The declared scope. From source, this is from the <c>scope</c> keyword only. | ||
| /// </summary> | ||
| internal DeclarationScope DeclaredScope => _scope; |
| allowThis: false, | ||
| addRefReadOnlyModifier: addRefReadOnlyModifier, | ||
| diagnostics: diagnostics); | ||
| diagnostics: diagnostics).Cast<SourceParameterSymbol, ParameterSymbol>(); |
There was a problem hiding this comment.
We're casting to
ParameterSymbolin many callers ofMakeParameters(). Where are we using the change toSourceParameterSymbol, and would it make sense to cast that case toSourceParameterSymbolinstead?
Is there a specific concern? This is a safe in-place cast. I intentionally changed return type to a more specific type in order to ensure that assumptions made in ParameterHelpers.ReportParameterErrors are justified.
| } | ||
| } | ||
| "; | ||
| var comp = CreateCompilation(new[] { source, UnscopedRefAttributeDefinition }, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseDll); |
|
@RikkiGibson, @dotnet/roslyn-compiler For the second review. |
|
@RikkiGibson, @dotnet/roslyn-compiler For the second review. |
| parameterRefKinds, | ||
| refKind: default, | ||
| returnType: default); | ||
| lambdaSymbol = createLambdaSymbol(); |
There was a problem hiding this comment.
It seems strange that we will sometimes create a lambda symbol which gets thrown away in order to produce information like the inferred return type or effective scope of parameters, and then later on presumably produce a "final" lambda symbol. Is this a sign that there's some design debt here which could be addressed in the future?
There was a problem hiding this comment.
Is this a sign that there's some design debt here which could be addressed in the future?
I do not think so. As long as this doesn't become a source of performance problem, that isn't something that I would worry about.
|
@AlekseyTs I can't find this behavior described in the spec. Should we document it? Did we confirm with LDM this is desirable? |
My understanding is that that we were propagating something already that didn't make sense in many scenarios. Triage determined that the behavior was not desirable and was classified as a bug. At this point I do not remember whether this behavior or the previous behavior was explicitly documented in the spec. If you believe we should have a different behavior, feel free to open a bug or bring this for LDM discussion. |
…4966) * Pass information to delegate inference through method symbol * Fix `DefaultParameterValue` test * Update caller info attribute tests * Enable lambda nullable reference types test * Parametrize IL snapshots * Test caller info attrs without default values * Fix merge of #63787 * Simplify by using a list pattern * Avoid creating a second `LambdaSymbol` * Add parameter scopes override * Keep comments at lines with expected diagnostics
Fixes #63565.
Closes #63078.