Remove --skip-unresolved from illink

[sbomer]

After the discussion around #90261 and #90469, I wanted to see if we could get rid of --skip-unresolved entirely by making the Resolve behavior always strict (error on resolution failure), and replacing just the callsites where we need to be resilient to failures (mainly when encountering type forwarders to assemblies that aren't present). I'm not sure yet if this is a good idea, so I'm curious what you think of the approach @vitek-karas @mrvoorhe.

When cecil needs to resolve an assembly as part of member resolution, it defers to our AssemblyResolver by calling Resolve directly. This change adds extension methods for exported types (unfortunately requiring some code to be duplicated from cecil) that allows resolving them with a call to a different Resolve overload that doesn't error on resolution failures.

With these changes, a few testcases that were intentionally written to test the behavior of missing references started failing, so I've changed the expected behavior for those.

[ghost]

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

Issue Details

---

After the discussion around #90261 and #90469, I wanted to see if we could get rid of --skip-unresolved entirely by making the Resolve behavior always strict (error on resolution failure), and replacing just the callsites where we need to be resilient to failures (mainly when encountering type forwarders to assemblies that aren't present). I'm not sure yet if this is a good idea, so I'm curious what you think of the approach @vitek-karas @mrvoorhe.

When cecil needs to resolve an assembly as part of member resolution, it defers to our AssemblyResolver by calling Resolve directly. This change adds extension methods for exported types (unfortunately requiring some code to be duplicated from cecil) that allows resolving them with a call to a different Resolve overload that doesn't error on resolution failures.

With these changes, a few testcases that were intentionally written to test the behavior of missing references started failing, so I've changed the expected behavior for those.

Author:	sbomer
Assignees:	-
Labels:	area-Tools-ILLink
Milestone:	-

[mrvoorhe]

Historically, I've been in favor of error on unresolved. While syncing up with upstream recently and having to revisit a lot of our resolving code I'm less certain of that view.

I'll brain dump some thoughts.

Unity - Mono vs IL2CPP player builds

Any behavior difference between Mono and IL2CPP is prone to being labeled an "IL2CPP issue" or a "Code stripping issue". Sometimes they are. Other times it's actually a horrible set of assemblies UnityLinker & IL2CPP have been given and types and methods are missing. A user doesn't realize there is an issue with the assemblies because the code paths with missing types or methods are never called and so there is never an error during mono player builds.

In many cases, using skip-unresolved=true allows a trimmed assembly to have the same build & runtime experience as a non-trimmed assembly. That is appealing.

Unity - Unresolved strubbing

Historically, in UnityLinker we hooked into the HandleUnresolvedMethod and HandleUnresolvedType calls and used that to record things that were missing so that UnityLinker could recreate these things. The history and motivation behind this predates me, but the impact was that problems with missing types and methods because runtime problems instead of build time problems. Which is more in line with the behavior you'd get if you had out of sync assemblies in a normal console app and ran it.

This scheme was unsustainable. It many cases you can't correctly recreate missing things. We won't be doing this going forward with CoreCLR bcl.

However, IL2CPP cannot handle missing types and method today. Using --skip-unresolved=true would be counter productive. It would mean instead of a decent error from UnityLinker you'd get a crash in IL2CPP. I just landed our sync up with the net7 illink and we are using --skip-unresolved=false when targeting IL2CPP for this reason.

We've never used --skip-unresolved=false

Using --skip-unresolved=false is new for us and I don't really know how well it's going to go. I do know that I've seen problematic code patterns that asset stores and packages perform. One example is that they will scan the Unity Editor assemblies with cecil and generate code based on the Editor assemblies. This is problematic when something is defined within #if UNITY_EDITOR and therefore won't exist when building a player.

UnityLinker & CoreCLR bcl

When I landed our net7 sync up the other day, I have UnityLinker using --skip-unresolved=true when targeting CoreCLR backend (i.e. no IL2CPP involved). This was to align with what you guys said was the default behavior of illink when ran from MSBuild.

[mrvoorhe]

I think my feeling is that if Microsoft wants to make illink fail on unresolved members, that would make life easier for us and our users in the long run.

I am nervous that the transition to get the Unity ecosystem aligned with erroring on unresolved members could be painful.

Given the unknowns, it may be wise to switch over to --skip-unresolved=false as a first step. Then later down the road if the transition has gone well, remove support for --skip-unresolved=true?

That's my 2 cents.

[sbomer]

Thanks for your thoughtful comments @mrvoorhe. @vitek-karas and I discussed it, and we shared your concern about the risk of this change, since we don't know which code might be depending on the --skip-unresolved true behavior. If we did change the default, we agreed it would be good to keep the flag as an opt-out, as you suggested.

The potential benefit of being stricter is that it could turn some runtime exceptions into build-time warnings/errors if you're trimming. But our philosophy behind trimming is that it should aim to warn about differences in behavior introduced by trimming, so producing these warnings isn't necessarily desirable for us.

Using --skip-unresolved=false is new for us and I don't really know how well it's going to go.
When I landed our net7 sync up the other day, I have UnityLinker using --skip-unresolved=true when targeting CoreCLR backend (i.e. no IL2CPP involved).

Could you clarify what the default was prior to your net7 sync? I wasn't sure because these two statements seemed contradictory - maybe there's a nuance I am missing.

[mrvoorhe]

Could you clarify what the default was prior to your net7 sync? I wasn't sure because these two statements seemed contradictory - maybe there's a nuance I am missing.

Yes, let me try again. It's hard to follow.

The situation today (Unity 2023.2) and all previous versions of Unity was:

• Trimming during IL2CPP Player Build - Our unresolved stubbing logic would kick in and recreate missing members. Methods would be recreated with a throw in them. This essentially gave IL2CPP builds a way to behave in roughly the same way that mono would.

• Trimming during Mono Player Build - Same as IL2CPP.

Essentially, Unity to-date has not treated resolution failures as build errors.

Now, a quick pause for context. With the sync up with illink, we removed our unresolved stubbing logic. There were a number of reasons.

1. It's impossible to handle all cases. For example, a missing type needs to have a certain base type. We can't know the base type.
2. Every few months a new case we didn't handle would pop up. Sometimes it was something we could wire up. Sometimes it was an impossible case.
3. This is a minor reason, but it would have taken additional work to rewire our unresolved stubbing into the latest illink. Some of the methods we hook into were moved.

The situation going forward with CoreCLR is (currently):

• Trimming during IL2CPP Player Build - We will use --skip-unresolved=false. With unresolved stubbing removed, IL2CPP will now encounter the same unresolved members if we let the build continue. IL2CPP can't handle this today. It will crash. So we decided to let the linkers --skip-unresolved=false ability report these problems.

• Trimming during CoreCLR Player Build - We will use --skip-unresolved=true to mimic the behavior of MSBuild + illink.

We are treading into new territory with the behavior during IL2CPP Player Builds. I don't know how it will go. I'm also not sure if having CoreCLR Player build behave differently is going to do be the best. We may end up adjusting the behavior before release.

@sbomer Does that make more sense?

[sbomer]

Thanks a lot, that clarifies it. Those defaults make sense to me. I'll be curious to see how the --skip-unresolved=false mode works out for IL2CPP Player builds. If that scenario hits errors due to forwarder assemblies that point to missing assemblies, maybe the behavior in this PR would be useful as a way to make the strict mode less strict for forwarders specifically.

I'm going to close this PR since I don't think we have a good reason to change the behavior at the moment.

[mrvoorhe]

@sbomer I'll keep you posted on how it goes.

I was thinking of special casing the two missing assemblies that the mscorlib shim references. I forget how, but I was ultimately able to get our linker tests green without having to resort to that.

Those two assemblies may not be enough. I may have to do what you suggest and treat missing references in forwarders specially. We'll see what happens.