Remove places where the compiler is swallowing exceptions #58870
Conversation
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/InternalUtilities/ExceptionUtilities.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/InternalUtilities/ExceptionUtilities.cs
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
So note the first commit here is actually @chsienki's PR #58843, and this was building off of it. So @CyrusNajmabadi and @AraHaan your feedback should go there. (This is a draft so we can get the second part working while @chsienki finishes his part.) |
There are a few places where compiler APIs are swallowing internal exceptions and returning null from APIs as a way to march on; this removes those places, and then removes our ReportAndCatch* APIs from the compiler layer entirely. We currently have no (cross platform) way of implementing those in a way that would allow for non-fatal error reporting. Closes dotnet#58375
jasonmalinowski
force-pushed
the
stop-swallowing-exceptions
branch
from
b2719a2 to
d23fe1e
Compare
There was a problem hiding this comment.
Everything but the open question https://github.com/dotnet/roslyn/pull/58870/files#r794881243 LGTM. Signing off with the assumption that will be resolved before merge
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs
Outdated
Show resolved
Hide resolved
jasonmalinowski
force-pushed
the
stop-swallowing-exceptions
branch
from
dfbd0ed to
45fbb55
Compare
There was a problem hiding this comment.
LGTM to merge once validation completes https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=5703440&view=results
jasonmalinowski
force-pushed
the
stop-swallowing-exceptions
branch
from
45fbb55 to
b250868
Compare
|
@RikkiGibson @333fred @mavasani Turned out that change did require a test baseline to be updated in one test, and required another small tweak. I've made the update and you can view the diff here since I force-pushed. We do have a test that confirms that nulls being returned from analyzers already produce diagnostics that it's bad, so no reason to report further exceptions there. The other test got a baseline update so we do have coverage on this path after all. I can't say the output of the exception in that case is pretty but if we want to tweak that further let's do so after this is in. |
If an analyzer threw an exception, we generate a message that tells you what diagnostic IDs the analzyer produces, so you know what to disable so you can work around the analyzer. If the analyzer then also were to throw exceptions while we ask it for it's SupportedDiagnostics, we were eating that exception. This includes that as well.
jasonmalinowski
force-pushed
the
stop-swallowing-exceptions
branch
from
b250868 to
496d572
Compare
|
Validation run is clean. Go for it @jasonmalinowski. |
ghost
added this to the 
There are a few places where compiler APIs are swallowing internal exceptions and returning null from APIs as a way to march on; this removes those places, and then removes our ReportAndCatch* APIs from the compiler layer entirely. We currently have no (cross platform) way of implementing those in a way that would allow for non-fatal error reporting.
Closes #58375