-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[TypeName] Nested types should respect MaxNode count #106334
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
Changes from 6 commits
6cdd040
da4df69
f3183e0
84c7fbc
3f93356
43082d5
815d8af
b90de5b
593b00d
2a56a1f
09e2901
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -49,7 +49,7 @@ private TypeNameParser(ReadOnlySpan<char> name, bool throwOnError, TypeNameParse | |||||
| { | ||||||
| if (throwOnError) | ||||||
| { | ||||||
| if (parser._parseOptions.IsMaxDepthExceeded(recursiveDepth)) | ||||||
| if (IsMaxDepthExceeded(parser._parseOptions, recursiveDepth)) | ||||||
| { | ||||||
| ThrowInvalidOperation_MaxNodesExceeded(parser._parseOptions.MaxNodes); | ||||||
| } | ||||||
|
|
@@ -67,13 +67,13 @@ private TypeNameParser(ReadOnlySpan<char> name, bool throwOnError, TypeNameParse | |||||
| // this method should return null instead of throwing, so the caller can get errorIndex and include it in error msg | ||||||
| private TypeName? ParseNextTypeName(bool allowFullyQualifiedName, ref int recursiveDepth) | ||||||
| { | ||||||
| if (!TryDive(ref recursiveDepth)) | ||||||
| if (!TryDive(_parseOptions, ref recursiveDepth)) | ||||||
| { | ||||||
| return null; | ||||||
| } | ||||||
|
|
||||||
| List<int>? nestedNameLengths = null; | ||||||
| if (!TryGetTypeNameInfo(ref _inputString, ref nestedNameLengths, out int fullTypeNameLength)) | ||||||
| if (!TryGetTypeNameInfo(_parseOptions, ref _inputString, ref nestedNameLengths, ref recursiveDepth, out int fullTypeNameLength)) | ||||||
| { | ||||||
| return null; | ||||||
| } | ||||||
|
|
@@ -147,14 +147,38 @@ private TypeNameParser(ReadOnlySpan<char> name, bool throwOnError, TypeNameParse | |||||
| { | ||||||
| _inputString = capturedBeforeProcessing; | ||||||
| } | ||||||
| else | ||||||
| { | ||||||
| // Every constructed generic type needs the generic type definition. | ||||||
| if (!TryDive(_parseOptions, ref recursiveDepth)) | ||||||
| { | ||||||
| return null; | ||||||
| } | ||||||
| // If that generic type is a nested type, this needs to be taken into account. Example: | ||||||
| // "Namespace.Declaring+NestedGeneric`1[GenericArg]" requires 5 TypeName instances: | ||||||
|
||||||
| /// Represents the total number of <see cref="TypeName"/> instances that are used to describe | |
| /// this instance, including any generic arguments or underlying types. |
And we have 5 instances.
The declaring type of the generic type definition and of the constructed generic type is the same object:
Do you suggest that because it's the same object, it should not be included?
Also, the implementation of GetNodeCount seems to have a bug - it is double-counting nested TypeName instances.
It does that on purpose: to include the nodes from generic type definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you suggest that because it's the same object, it should not be included?
Yes. If it is the same object, it is the same instance and it should be only counted once.
We have multiple ways to get to that instance through the object model. I do not think that it should be a reason to count it multiple times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. If it is the same object, it is the same instance and it should be only counted once
You are right. I've changed the implementation and the tests, PTAL.


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this use
checkedarithmetic to avoid silent overflows?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because we can not parse more than
Int32.MaxValuenodes becauseTypeNameParseOptions.MaxNodesis also anInt32.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeName.Parseis not the only way to create new type names after the last PR. Try something like this:It is going to hit the overflow in this code.