Refactor missing reference errors to allow not throwing for all cases#50437
Conversation
davidwrighton
commented
Mar 30, 2021
davidwrighton
commented
- Build the concept of a cacheable resolution failure
- Plumb it through the Ecma type loader and the required public api surfaces
- Use it within the Mibc parser to avoid throwing
- Build the concept of a cacheable resolution failure - Plumb it through the Ecma type loader and the required public api surfaces - Use it within the Mibc parser to avoid throwing
|
@dotnet/crossgen-contrib The current state of pgo data loading was quite difficult to debug due to the excessive amount of exception throwing. This change allows typesystem users to disable all those exceptions as desired. |
| typeName = fullName.Substring(split + 1); | ||
| } | ||
| return module.GetType(namespaceName, typeName, throwIfNotFound); | ||
| return module.GetType(namespaceName, typeName, throwIfNotFound ? NotFoundBehavior.Throw : NotFoundBehavior.ReturnNull); |
There was a problem hiding this comment.
Would it make sense to propagate the new enum up the call graph instead of decoding the bool "in the middle"?
There was a problem hiding this comment.
Plausibly, but this change is already quite large enough, and very few components other than the internals of the typesystem actually can do anything useful with ResolutionFailure objects. So I don't think we need to perturb the api of the typesystem more than I already have.
|
|
||
| public PropertySignature ParsePropertySignature() | ||
| { | ||
| // As PropertySignature is a struct, we cannot return null |
There was a problem hiding this comment.
Hmm, I have a hard time understanding the exact semantics of this check. Does that mean that we can only use non-Throw signature parser modes when we know upfront that we won't be parsing any property signatures? Why can't we just throw when ParseType returns null?
There was a problem hiding this comment.
The principle is that if one only throws if ParseType returns null, its easy for a bug like using a non-throwing parse mode to be exist and require an obscure test case or worse customer scenario to actually encounter the bug. By forcing the throw eagerly, it becomes easier to find the bug of using the ParsePropertySignature method incorrectly. I didn't adjust the api to allow for this, as there aren't any uses of this function in the current repo, so I don't have a good way to be confident that users will be served well by any api I come up with.
| /// or <see cref="MethodSignature"/>). | ||
| /// </summary> | ||
| public abstract Object GetObject(int token); | ||
| public abstract Object GetObject(int token, NotFoundBehavior notFoundBehavior); |
There was a problem hiding this comment.
Can we give this one a default value (throw) so that we can limit the number of places where this needs to be passed?
There was a problem hiding this comment.
Yes. I always start my refactor without the default param, just to make sure I've made a decision at all points, but as you say, it hits a bunch of stuff which could benefit from a default param. I've put that in.
| { | ||
| public class ResolutionFailure | ||
| { | ||
| private enum FailureType |
There was a problem hiding this comment.
Wondering if it would be easier to just have a single field in this that is the exception instance.
sealed class ResolutionFailure
{
private readonly Exception _exception;
public ResolutionFailure(Exception ex) => _exception = ex;
public void Throw() => throw _exception;
}We would need to update ThrowHelper so that we have CreateXXX methods along with ThrowXXX methods on it.
Then the usage is "new ResolutionFailure(ThrowHelper.CreateFileNotFoundException(...));`
There was a problem hiding this comment.
I considered that, but unfortunately, as these ResolutionFailure objects are persistent, it can result in multiple throws with the exact same exception in them. While this does work, it results in stack traces in the exception being somewhat nondeterministic, which I very much disapprove of.
This was missed in #50437. Wonder if we should have just introduced a new overload of GetType that returns `object`. This "return resolution failure that we then need to not forget to check" looks like a potential bug farm. ```csharp public MetadataType GetType(string nameSpace, string name, bool throwIfNotFound = true) { /* the obvious implementation that calls the virtual method */ } public abstract object GetType(string nameSpace, string name, NotFoundBehavior notFoundBehavior) ```
This was missed in #50437. Wonder if we should have just introduced a new overload of GetType that returns `object`. This "return resolution failure that we then need to not forget to check" looks like a potential bug farm. ```csharp public MetadataType GetType(string nameSpace, string name, bool throwIfNotFound = true) { /* the obvious implementation that calls the virtual method */ } public abstract object GetType(string nameSpace, string name, NotFoundBehavior notFoundBehavior) ```
ghost
locked as resolved and limited conversation to collaborators
4 participants
