Improve few diagnostics #52047
Improve few diagnostics #52047
Conversation
|
Closing and re-opening for a new build. |
jcouv
added
the
Community
|
Would you mind going through the test cases that use these diagnostics and updating the messages? |
Sure. Let me know if you want to update the enum member names as well. |
| @@ -547,7 +547,7 @@ | |||
| <value>Inconsistent accessibility: parameter type '{1}' is less accessible than delegate '{0}'</value> | |||
| </data> | |||
| <data name="ERR_BadVisBaseClass" xml:space="preserve"> | |||
| <value>Inconsistent accessibility: base class '{1}' is less accessible than class '{0}'</value> | |||
| <value>Inconsistent accessibility: base type '{1}' is less accessible than type '{0}'</value> | |||
There was a problem hiding this comment.
type [](start = 44, length = 4)
I do not think we should make this change. A record class is a class.
| @@ -1138,7 +1138,7 @@ | |||
| <value>Partial declarations of '{0}' have conflicting accessibility modifiers</value> | |||
| </data> | |||
| <data name="ERR_PartialMultipleBases" xml:space="preserve"> | |||
| <value>Partial declarations of '{0}' must not specify different base classes</value> | |||
| <value>Partial declarations of '{0}' must not specify different base types</value> | |||
There was a problem hiding this comment.
types [](start = 73, length = 5)
I do not think we should make this change. A record class is a class and this error isn't about an interface in the base list, but specifically about the class type.
| @@ -1276,7 +1276,7 @@ | |||
| <value>Duplicate constraint '{0}' for type parameter '{1}'</value> | |||
| </data> | |||
| <data name="ERR_ClassBoundNotFirst" xml:space="preserve"> | |||
| <value>The class type constraint '{0}' must come before any other constraints</value> | |||
There was a problem hiding this comment.
class [](start = 15, length = 6)
I do not think we should make his change. This error is specifically about class types (record classes are classes), not about interface types, for example.
|
@AlekseyTs This makes diagnostics consistent with the changes already done in #46341. As a specific example (there are many): - Circular base class dependency involving '{0}' and '{1}'
+ Circular base type dependency involving '{0}' and '{1}' |
Didn't go through the changes in it, so it is quite possible that some or even all changes there make sense, or made sense at the time. I don't think there is a goal to get to "consistency" where every mention of a "class" is replaced with "type". Especially that now we have record classes and record structures, I don't believe there is a good reason to avoid referring to record classes as classes in cases when "record-ness" of the type isn't really important, by comparison to other aspects of the type. |
|
Got your point. But this probably means some of the changes in #46341 shouldn't have been made? |
Quite possible, however, it looks like some of the messages modified in this PR were looked at in context of that PR. They were not touched at the end. There are commits referring to ERR_BadVisBaseClass and ERR_ClassBoundNotFirst, and ERR_PartialMultipleBases was left unchanged based on my feedback (#46341 (comment)). |
|
Thanks for the additional info @AlekseyTs! That makes much sense. |
I did part of the changes I suggested in #48286. The others in the issue aren't really important I think. But let me know if you think otherwise.
Fixes #48286