What is the expected behavior when a reference is missing?

[mrvoorhe]

I've been fighting with some tests in our UnityLinker suite that verify our behavior when a reference is missing. What I need to know to get unblocked is what the design intend is of illink. I've had a hard time figuring out what the excepted behavior is. The TryResolve calls and probing logic in AssemblyResolver lead me to believe there are times when an unresolved assembly shouldn't be treated as an error. However, after porting some of our tests to ilink's suite I see that an unresolved reference is fatal regardless of whether or not it's in a used code path.

Here are the two tests.

MissingReferenceInUsedCodePath - This test passes as expected. An exit code of 1 is returned.

MissingReferenceInUnusedCodePath - This test is where I'm unclear what the expected behavior is. Should the linker fail or pass in this case? Currently it fails. DiscoverDynamicCastableImplementationInterfaces triggers a resolve failure which leads to the linker failing. SweepAssemblyReferences also leads to a resolve failure.

@vitek-karas @marek-safar ?

[ghost]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

I've been fighting with some tests in our UnityLinker suite that verify our behavior when a reference is missing. What I need to know to get unblocked is what the design intend is of illink. I've had a hard time figuring out what the excepted behavior is. The TryResolve calls and probing logic in AssemblyResolver lead me to believe there are times when an unresolved assembly shouldn't be treated as an error. However, after porting some of our tests to ilink's suite I see that an unresolved reference is fatal regardless of whether or not it's in a used code path.

Here are the two tests.

MissingReferenceInUsedCodePath - This test passes as expected. An exit code of 1 is returned.

MissingReferenceInUnusedCodePath - This test is where I'm unclear what the expected behavior is. Should the linker fail or pass in this case? Currently it fails. DiscoverDynamicCastableImplementationInterfaces triggers a resolve failure which leads to the linker failing. SweepAssemblyReferences also leads to a resolve failure.

@vitek-karas @marek-safar ?

Author:	mrvoorhe
Assignees:	-
Labels:	linkable-framework, area-Tools-ILLink
Milestone:	-

[sbomer]

In practice we always pass --skip-unresolved true when running through the MSBuild integration, so TryResolve and Resolve will behave identically. To answer your question, I think the expected behavior is to tolerate resolution failures in unused code when --skip-unresolved is true but not otherwise.

The reason we want to tolerate resolution errors is that often apps include the mscorlib.dll shim (providing type forwarders that allows the use of dependencies which target .NET Framework). mscorlib.dll has some forwarders that are unused by most apps, such as forwarders to System.Security.Permissions which isn't part of netcoreapp, and these would lead to resolution failures without --skip-unresolved true.

We probably have quite inconsistent use of Resolve vs TryResolve in the codebase. I'd be in favor of getting rid of one or the other.

[mrvoorhe]

mscorlib.dll has some forwarders that are unused by most apps, such as forwarders to System.Security.Permissions which isn't part of netcoreapp

😂 That is the exact issue giving me grief in some of ours tests.

In practice we always pass --skip-unresolved true when running through the MSBuild integration,

I didn't realize that. That's good to know.

I think the expected behavior is to tolerate resolution failures in unused code when --skip-unresolved is true but not otherwise.

That definitely works. And if it were not for DiscoverDynamicCastableImplementationInterfaces and SweepAssemblyReferences the linker would tolerate unresolved references in unused code paths even with skip unresolved is false.

DiscoverDynamicCastableImplementationInterfaces has a note in it that says it would be nice if it didn't load all references

// We could potentially avoid loading all references here: https://github.com/dotnet/linker/issues/1788

Which is why I wasn't sure on the expected behavior.

[mrvoorhe]

We probably have quite inconsistent use of Resolve vs TryResolve in the codebase. I'd be in favor of getting rid of one or the other.

I was trying to figure out the intent as well. if there is a desire to tolerate unresolved references in unused code paths, then I see the value of having TryResolve and Resolve. In UnityLinker I started using TryResolve anytime we search for types, otherwise I'd use Resolve.

[mrvoorhe]

I'm going to polish off the tests in this PR to assert the current behavior. I'll leave it to you guys to figure out whether or not the current behavior is the desired behavior.

[vitek-karas]

#90469 is basically discussing the same problem.
And the proposed #90512 would make this cleaner. We would probably still need some tests to opt-out of it - but the default behavior even in tests would match the expected default behavior of the product (SDK).

[mrvoorhe]

I updated the failing test to assert the current behavior. If CI is green the PR is ready to merge.