Don't crash the trim analyzer if it finds unrecognized nodes in the input

[vitek-karas]

New versions of the compiler will introduce new nodes and values. The analyzer can never be 100% in sync with the compiler, so it needs to be able to gracefully handle nodes it doesn't know anything about.

Change the several throws to just Debug.Fail. For end-users if we hit unrecognized node, we will effectively ignore that part of the code. So not 100% precise, but the analyzer will never be 100% regardless.

This is in response to #88684, but we can't add tests for it yet because the necessary compiler changes are in Preview 6, the repo is still on Preview 5.

[ghost]

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

Issue Details

---

New versions of the compiler will introduce new nodes and values. The analyzer can never be 100% in sync with the compiler, so it needs to be able to gracefully handle nodes it doesn't know anything about.

Change the several throws to just Debug.Fail. For end-users if we hit unrecognized node, we will effectively ignore that part of the code. So not 100% precise, but the analyzer will never be 100% regardless.

This is in response to #88684, but we can't add tests for it yet because the necessary compiler changes are in Preview 6, the repo is still on Preview 5.

Author:	vitek-karas
Assignees:	vitek-karas
Labels:	area-Tools-ILLink
Milestone:	8.0.0

[ghost]

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar
See info in area-owners.md if you want to be subscribed.

Issue Details

---

New versions of the compiler will introduce new nodes and values. The analyzer can never be 100% in sync with the compiler, so it needs to be able to gracefully handle nodes it doesn't know anything about.

Change the several throws to just Debug.Fail. For end-users if we hit unrecognized node, we will effectively ignore that part of the code. So not 100% precise, but the analyzer will never be 100% regardless.

This is in response to #88684, but we can't add tests for it yet because the necessary compiler changes are in Preview 6, the repo is still on Preview 5.

Author:	vitek-karas
Assignees:	vitek-karas
Labels:	linkable-framework, area-Tools-ILLink
Milestone:	8.0.0

[sbomer]

What are your thoughts on making this a warning instead? It would be nice to leave some feedback channel for the unimplemented cases.

[vitek-karas]

I'm on the edge on this one:

• It is non-actionable to the end user - so we're basically pushing our mess to the end user to help us to clean it... not nice
• I'd love to know if this happens obviously

Currently I'm slightly leaning towards silence. Basically the thinking is:

• We have to keep an eye on all language/runtime changes anyway to keep illink and NativeAOT in sync with that support. We dropped the ball on InlineArrays for some reason, but that should be easy to fix (as a process/awareness). A counter example is UnsafeAccess which was/is handled well. So we will write tests for illink/AOT for this and the analyzer will get some of that code to run on. And we will "know" about it so probably try it as well.
• I think there's basically two types of changes the compiler might do
  • small improvements like this one which are very likely explicit opt-in (as in the user has to write the new code shape) - so not having support for it is probably not going to hurt anything. And if it does we'll hear about it.
  • large changes (like async) - but we will be well aware of those anyway, so we will react soon.

The argument against the warning is: If this happens to you (maybe because of a nuget dependency for example), there's nothing you can do other than file the issue (which most people won't bother with anyway). And there's no "Fixing it" either, you have to figure out the NoWarn and suppress it. But that's likely going to stay in your codebase forever, and so the next time this happens, you won't know and won't tell us anyway.

[sbomer]

I agree that for large compiler features we would probably be able to anticipate this. I also see the potential problem you point out with NoWarn, especially since a suppression would prevent future warnings about different unimplemented node types. I still think we would get good feedback from a warning; the warning could say "please file an issue".

For smaller cases like this, I don't think we would have found out about this hole until much later without the feedback channel. Another idea is to run the debug version of the analyzer on runtime bits (not sure if the performance would be acceptable - maybe we would do it in certain ci legs only)?

@agocke in case you have other ideas.

[agocke]

What are your thoughts on making this a warning instead?

Warnings are errors for customers. Can't do that.

I'd instead do something like Debug.Assert or somehow run in a different configuration locally. That would let us see the failure in runtime dogfooding, but wouldn't cause problems for customers. Just looked at the diff and saw Debug.Fail. We should just ensure that the analyzer is running in Debug in some scenario.

[vitek-karas]

Actually I think it would be valuable to have a CI leg which runs as many analyzers/sourcegenerators as we can in Debug mode. Ideally all the analyzers which are built by the runtime repo (but I don't know how the codeflow is setup).

I filed #88901 to track that.

[agocke]

There's also something I remember called a "non-fatal Watson" which is supposed to send some sort of Watson failure, without crashing or throwing. I can go searching for what that is.