Tolerate assembly resolution errors by sbomer · Pull Request #260 · dotnet/linker

sbomer · 2018-02-23T00:25:50Z

Previously, we swallowed AssemblyResolutionException in a few places
in various steps. We need an option to be generally more tolerant of
AssemblyResolutionExceptions, because there can be cases during
portable publish in which referenced assemblies are not present during
publish because they're part of the shared framework.

Instead of handling AssemblyResolutionException in every place in the
linker that tries to resolve methods/types/etc, this change adds an
option that enables the resolver to return null when an assembly is
not found. The null result is also cached to avoid multiple lookups.

Only a few places in the linker need to be modified to handle null
results, and the existing places where we used to catch
AssemblyResolutionException now only need to deal with a null result
instead. The downside of this approach is that these places cannot
distinguish between types not found in an existing assembly, and
assemblies that were not found.

The new behavior is grouped under the option --skip-unresolved, which
already handles unresolved types and methods in some cases. When this
option is enabled, a warning is logged, and no exception is thrown.

@erozenfeld, please review

Previously, we swallowed AssemblyResolutionException in a few places in various steps. We need an option to be generally more tolerant of AssemblyResolutionExceptions, because there can be cases during portable publish in which referenced assemblies are not present during publish because they're part of the shared framework. Instead of handling AssemblyResolutionException in every place in the linker that tries to resolve methods/types/etc, this change adds an option that enables the resolver to return null when an assembly is not found. The null result is also cached to avoid multiple lookups. Only a few places in the linker need to be modified to handle null results, and the existing places where we used to catch AssemblyResolutionException now only need to deal with a null result instead. The downside of this approach is that these places cannot distinguish between types not found in an existing assembly, and assemblies that were not found. The new behavior is grouped under the option --skip-unresolved, which already handles unresolved types and methods in some cases. When this option is enabled, a warning is logged, and no exception is thrown.

Introduce a SkipUnresolved attribute

marek-safar · 2018-02-23T12:35:31Z


 		readonly Dictionary<string, AssemblyDefinition> _assemblies;
+		readonly HashSet<string> _unresolvedAssemblies;
+		bool _ignoreUnresolved = false;


redundant initialization

marek-safar · 2018-02-23T12:42:26Z

+					if (!_ignoreUnresolved) {
+						throw;
+					}
+					Console.WriteLine($"warning: unresolved assembly {name.Name}");


Use LinkContext::LogMessage instead

marek-safar · 2018-02-23T12:43:15Z

 		public AssemblyResolver (Dictionary<string, AssemblyDefinition> assembly_cache)
 		{
 			_assemblies = assembly_cache;
+                        _unresolvedAssemblies = new HashSet<string>();


I'd prefer to delay initialization of this to the catch block

marek-safar · 2018-02-23T12:43:40Z

+			AssemblyDefinition asm = null;
+			if (!_assemblies.TryGetValue (name.Name, out asm) && !_unresolvedAssemblies.Contains (name.Name)) {
+				try
+				{


Follow existing coding style

marek-safar · 2018-02-23T12:59:49Z

-			set { _ignoreUnresolved = value; }
+			set {
+				_ignoreUnresolved = value;
+				_resolver.IgnoreUnresolved = value;


I don't like this, _resolver could be a shared/global instance and you are changing it under the hood

- follow coding conventions - use LinkContext::LogMessage instead of Console.WriteLine - delay initialization of unresolved assembly cache to catch block - keep AssemblyResolver.IgnoreUnresolved independent of LinkContext.IgnoreUnresolved - remove redundant initialization

sbomer · 2018-02-23T18:16:14Z

@marek-safar I addressed your feedback, thanks!

marek-safar · 2018-02-23T19:23:17Z

build

sbomer · 2018-02-23T19:29:28Z

Hold off on merging this for a minute - just discovered a bug. I need to check for null in AssemblyResolver.Dispose.

Edit: fixed now.

marek-safar · 2018-02-23T19:30:24Z

 			}

 			_assemblies.Clear ();
+			_unresolvedAssemblies.Clear ();


_unresolvedAssemblies can be null, setting it to null should do it

marek-safar · 2018-02-23T19:42:05Z

+			set { _context = value; }
+		}
+
 		public override AssemblyDefinition Resolve (AssemblyNameReference name, ReaderParameters parameters)


I think it'd be better to pass LinkContext Context as the parameter

The problem is that the Resolve method signature is part of IAssemblyResolver, defined in cecil. I want warnings to be logged when an assembly can't be resolved while trying to resolve TypeReferences to TypeDefinitions using cecil, for example. Do you have any suggestions?

marek-safar · 2018-02-23T19:42:32Z

 			}

 			_assemblies.Clear ();
+			if (_unresolvedAssemblies != null)


Just set it to null

* Tolerate assembly resolution errors Previously, we swallowed AssemblyResolutionException in a few places in various steps. We need an option to be generally more tolerant of AssemblyResolutionExceptions, because there can be cases during portable publish in which referenced assemblies are not present during publish because they're part of the shared framework. Instead of handling AssemblyResolutionException in every place in the linker that tries to resolve methods/types/etc, this change adds an option that enables the resolver to return null when an assembly is not found. The null result is also cached to avoid multiple lookups. Only a few places in the linker need to be modified to handle null results, and the existing places where we used to catch AssemblyResolutionException now only need to deal with a null result instead. The downside of this approach is that these places cannot distinguish between types not found in an existing assembly, and assemblies that were not found. The new behavior is grouped under the option --skip-unresolved, which already handles unresolved types and methods in some cases. When this option is enabled, a warning is logged, and no exception is thrown. * Attempt to fix test failure Introduce a SkipUnresolved attribute * Address PR feedback - follow coding conventions - use LinkContext::LogMessage instead of Console.WriteLine - delay initialization of unresolved assembly cache to catch block - keep AssemblyResolver.IgnoreUnresolved independent of LinkContext.IgnoreUnresolved - remove redundant initialization * Handle null _unresolvedAssemblies in Dispose method Commit migrated from dotnet/linker@2dee7d0

sbomer requested a review from marek-safar as a code owner February 23, 2018 00:25

sbomer force-pushed the handleUnresolvedAssemblies branch from 2a05c2d to 88653c9 Compare February 23, 2018 03:39

Attempt to fix test failure

d47d9d7

Introduce a SkipUnresolved attribute

sbomer force-pushed the handleUnresolvedAssemblies branch from 88653c9 to d47d9d7 Compare February 23, 2018 04:28

marek-safar requested changes Feb 23, 2018

View reviewed changes

Handle null _unresolvedAssemblies in Dispose method

8d9de2c

marek-safar requested changes Feb 23, 2018

View reviewed changes

marek-safar approved these changes Feb 23, 2018

View reviewed changes

marek-safar merged commit 2dee7d0 into dotnet:master Feb 23, 2018

sbomer mentioned this pull request Feb 26, 2018

Enable link during portable publish #263

Merged

sbomer mentioned this pull request May 29, 2020

Support DynamicDependencyAttribute #1215

Merged

Conversation

sbomer commented Feb 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbomer commented Feb 23, 2018

Uh oh!

marek-safar commented Feb 23, 2018

Uh oh!

sbomer commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sbomer commented Feb 23, 2018 •

edited

Loading