C#: Enable nullability of Semmle.Extraction.CSharp.Standalone#4115
Conversation
tamasvajk
force-pushed
the
feature/nullability-extraction-csharp-standalone
branch
2 times, most recently
from
66fd9d7 to
69050fa
Compare
| public static AssemblyInfo MakeFromAssembly(Assembly assembly) => new AssemblyInfo() { Valid = true, Filename = assembly.Location, Id = assembly.FullName }; | ||
| public static AssemblyInfo MakeFromAssembly(Assembly assembly) | ||
| { | ||
| if (assembly.FullName is null) |
There was a problem hiding this comment.
this is a slight change in functionality. FullName can be null, now we're explicitly throwing an exception, previously the first access would have thrown (a different type of exception).
| { | ||
| var sortedReferences = usedReferences. | ||
| Select(r => assemblyCache.GetAssemblyInfo(r.Key)). | ||
| Where(r => r != AssemblyInfo.Invalid). |
There was a problem hiding this comment.
This validity check was missing before.
csharp/extractor/Semmle.Extraction.CSharp.Standalone/CsProjFile.cs
Outdated
Show resolved
Hide resolved
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
This is excellent, Tamas. Nice refactorings.
csharp/extractor/Semmle.Extraction.CSharp.Standalone/CsProjFile.cs
Outdated
Show resolved
Hide resolved
csharp/extractor/Semmle.Extraction.CSharp.Standalone/BuildAnalysis.cs
Outdated
Show resolved
Hide resolved
| // Fast path if we've already seen this before. | ||
| if (failedAssemblyInfoIds.Contains(id)) | ||
| return AssemblyInfo.Invalid; | ||
| throw new AssemblyLoadException(); |
There was a problem hiding this comment.
It feels like we should have more information in this exception, for example pass id to the constructor of AssemblyLoadException.
There was a problem hiding this comment.
I can add it, but it would not be used at the moment. Previously, we had the static AssemblyInfo.Invalid, which also didn't have any more information. The reason why I changed it to an exception is that AssemblyInfo.Invalid was basically a partially constructed AssemblyInfo, which had a lot of uninitialized properties.
| } | ||
| catch (AssemblyLoadException) | ||
| { | ||
| } |
There was a problem hiding this comment.
It does feel like we should record this failure somewhere. Why not call UnresolvedReference?
There was a problem hiding this comment.
We can't call UnresolvedReference because at this point we're not in a project context, so we wouldn't be able to pass the second parameter.
At the same time, I'm not sure this catch is required at all. We're iterating over usedReferences, which should all be already resolved once, so I don't think this can fail. Also, we had no equality check here previously for the AssemblyInfo.Invalid either.
I'm adding logging to this catch.
tamasvajk
force-pushed
the
feature/nullability-extraction-csharp-standalone
branch
from
096ee6b to
558a012
Compare
3 participants
No description provided.