Update framework detection logic to not rely on throwing/catching NRE #3714
Conversation
| @@ -32,7 +32,7 @@ internal AssemblyMetadataProvider(IFileHelper fileHelper) | |||
| } | |||
|
|
|||
| /// <inheritdoc /> | |||
| public FrameworkName GetFrameWork(string filePath) | |||
| public FrameworkName GetFrameworkName(string filePath) | |||
There was a problem hiding this comment.
Somehow unrelated but I got confused by the API name compared to its return type when fixing the auto-detection algorithm so I also fixed the name.
| framework); | ||
| } | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
Try/catch was only catching NRE when finalFx is null which happens only when all sources provided are null. DetermineFrameworkName and Framework.FromString are having inner try/catch.
| @@ -133,35 +133,24 @@ public Architecture AutoDetectArchitecture(IList<string?>? sources, Architecture | |||
| public Framework AutoDetectFramework(IList<string?>? sources, out IDictionary<string, Framework> sourceToFrameworkMap) | |||
| { | |||
| sourceToFrameworkMap = new Dictionary<string, Framework>(); | |||
| Framework framework = Framework.DefaultFramework; | |||
|
|
|||
| if (sources == null || sources.Count == 0) | |||
There was a problem hiding this comment.
I am not filtering null sources here because the simplest solution is to actually return Framework instead of FrameworkName on DetermineFramework. This way I can solve the "problem" of instances equality.
| return framework; | ||
| } | ||
|
|
||
| private FrameworkName? DetermineFrameworkName(IEnumerable<string?> sources, out IDictionary<string, Framework> sourceToFrameworkMap, out bool conflictInFxIdentifier) | ||
| private Framework DetermineFramework(IEnumerable<string?> sources, out IDictionary<string, Framework> sourceToFrameworkMap, out bool conflictInFxIdentifier) |
There was a problem hiding this comment.
Potentially we could move this as local function of AutoDetetFramework.
Evangelink
force-pushed
the
prevent-expected-NRE
branch
from
43c490b
to
6128a31
Compare
Evangelink
changed the title
Evangelink
changed the title
Description
Update framework detection logic to not rely on throwing/catching NRE. When running some tests locally in Debug we had
Debug.Assertfailures, in CI/release the behavior was the same as before because assert are ignored.