Disallow declaration of indexers in absence of proper DefaultMemberAttribute. #75099
Conversation
…Attribute is missing. Fixes dotnet#75032.
dotnet-issue-labeler
bot
added
the
untriaged
| @@ -1713,7 +1713,7 @@ internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, r | |||
|
|
|||
| AddSynthesizedAttribute(ref attributes, compilation.TrySynthesizeAttribute( | |||
| WellKnownMember.System_Reflection_DefaultMemberAttribute__ctor, | |||
| ImmutableArray.Create(defaultMemberNameConstant))); | |||
| ImmutableArray.Create(defaultMemberNameConstant), isOptionalUse: true)); | |||
There was a problem hiding this comment.
I guess my question is, why is this ok to not emit? #Resolved
There was a problem hiding this comment.
It is not ok, but this is what we were doing for many years. If we start reporting an error (like VB), we are going to break runtime build, at least. I don't think this is worth it. The scenario is very uncommon.
There was a problem hiding this comment.
My feeling is, this (where the error would be reported) is a test-only component of NativeAOT that is fairly new. It seems likely to me that it should be including this attribute, and not having it could end up masking a bug. /cc @agocke
There was a problem hiding this comment.
What’s the actual proposal? That the compiler emits an error for this case and the runtime fixes the test code?
I’m fine with that.
Is it possible that anyone could hit the error without hitting the assert? If so the question really is what happens if you get a bug from windows or office about the new error. Will you change the compiler or tell them to fix their code? Whatever is the answer to that question is the path I would take.
There was a problem hiding this comment.
That the compiler emits an error for this case and the runtime fixes the test code?
That's what I would do, yes.
There was a problem hiding this comment.
Is it possible that anyone could hit the error without hitting the assert?
Sure. The runtime does right now 🙂.
AlekseyTs
changed the title
AlekseyTs
changed the title
|
@dotnet/roslyn-compiler For the second review |
| } | ||
| } | ||
|
|
||
| if (Indexers.FirstOrDefault() is PropertySymbol indexerSymbol) | ||
| { | ||
| Binder.GetWellKnownTypeMember(DeclaringCompilation, WellKnownMember.System_Reflection_DefaultMemberAttribute__ctor, diagnostics, indexerSymbol.TryGetFirstLocation() ?? GetFirstLocation()); |
There was a problem hiding this comment.
This is a defense in depth, theoretically the API can return null, even though we don't expect it to. This pattern is also used elsewhere in this function.
…terns * upstream/main: (267 commits) Support long chains of `else if` statements (dotnet#74317) Update dependencies from https://github.com/dotnet/source-build-externals build 20240930.2 Fix the path to the proposal (dotnet#75302) Fix TSA tooling (dotnet#75307) Clarify the bug template to request a code snippet (dotnet#75306) Bump razor for serialization changes (dotnet#75282) Disallow declaration of indexers in absence of proper DefaultMemberAttribute. (dotnet#75099) stoub use ref Simpler Simplify Switch to a threadlocal storage to prevent locks add comment don't mess with user caret in smart rename Update LanguageServer references Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2548898 Use common helper method Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2548278 Field-backed properties: additional tests (dotnet#75283) Revert "Updates content exclusion for on-the-fly-docs (dotnet#75172)" (dotnet#75279) (dotnet#75284) ...
Fixes #75032.