Skip to content
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

Fix handling of not found exported types #50845

Merged
merged 1 commit into from
Apr 8, 2021

Conversation

MichalStrehovsky
Copy link
Member

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.

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)
```
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@MichalStrehovsky
Copy link
Member Author

Looks like David is out - @dotnet/crossgen-contrib could someone have a look?

Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proximate fix looks good to me. I defer to David to comment on the broader design considerations you posited.

@MichalStrehovsky
Copy link
Member Author

I defer to David to comment on the broader design considerations you posited.

I'll make sure to ask that question again when this landmine explodes :)

@MichalStrehovsky MichalStrehovsky merged commit ae820d0 into main Apr 8, 2021
@MichalStrehovsky MichalStrehovsky deleted the MichalStrehovsky-patch-1 branch April 8, 2021 07:13
@ghost ghost locked as resolved and limited conversation to collaborators May 8, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants