Roslyn: Can we analyze function in try block to suggest exception type for catch?

[Thaina]

I think intellisense from roslyn should suggest possible exception that could be thrown from try block when we want exception type in catch(

At least for direct instance function and static function. Is it possible?

[CyrusNajmabadi]

We could do this if the symbols that were called have proper documentation comments. However, most of them won't, so that could be problematic.

[Thaina]

@CyrusNajmabadi What I suggest is, roslyn should analyze syntax tree of all function called in try block to the deepest level it could, View the type that was used. Filter only exception. Then listing it when we type the word catch. Not just looking at documentation comment

[CyrusNajmabadi]

This would be a very inaccurate analysis. What would we do if you called into an interface method? What about if you call into anything from metadata? We would end up filtering out viable exceptions that you may want to complete to.

We tend to under-filter rather than over-filter. over-filtering is very risky and can lead to a bad experience.

[Thaina]

@CyrusNajmabadi I would be left that out as limitation. And by definition of interface we could implement new object with it and throw new kind of exception so it never be able to know beforehand

That's why I state that

At least for direct instance function and static function

[alrz]

@CyrusNajmabadi

What about something along the lines of https://github.com/dotnet/roslyn/issues/15573 to suggest <exception> doc comments?

[CyrusNajmabadi]

At least for direct instance function and static function

This feels extremely contrived. In what circumstances will this actually happen? The amount of code that:

1. Is fully analyzable at a source level.
2. Never calls into metadata
3. Never makes an interface/virtual dispatch
4. Still throws some interesting exceptions

is going to be super small. I'd like to see the actual scenario we're trying to improve here as i imagine the work here would almost never end up benefiting people.

[Thaina]

@CyrusNajmabadi I think you are over thinking

It not that it will never analyze whole block if just one interface exist. Instead, it will analyze everything as deep as possible and gather as much as possible to filter out exception. It just skip on virtual or abstract or interface function

Consider this

```C#
try
{
var i = int.Parse(text); // Some exception here
if(i < 1)
{
var req = await HttpWebRequest.CreateHttp("GET",url);
using(var resp = req.GetResponse())
i = int.Parse(resp.GetResponseStream().ReadToEnd()); // might not work with TextReader because it is abstract but still analyze GetStream normally
}

IEnumerable<int> numbers = Enumerable.Range(0,i);
// numbers is interface OK, we can't analyze GetCurrent and Movenext but?
numbers = numbers.Except(new[] { 0,1,2 }); // Except is static! We can analyze Except

}
catch( // a bunch of exceptions could be able to catch here
```

As you can see

[CyrusNajmabadi]

req.GetResponse() is virtual. No clue what it may throw.
Same with GetResponseStream.
Same with ReadToEnd.
'Except' is static. Except (har har) it's in metadata. So we can't actually analyze it.

The above code could throw tons of different exceptions that we have no clue about.

[Thaina]

@CyrusNajmabadi I think we could analyze metadata. We need to to through it in debugger anyway

Well right, no virtual or interface. But I think I consider that it could analyze metadata so it should cover large enough

The above code could throw tons of different exceptions that we have no clue about.

That's what I think we should know

[CyrusNajmabadi]

Ok. So you analyze metadata now. Say it's one of those virtual methods i mentioned. You'll have to go find all the potential overrides of them. Examine all of them including what they call transitively. Including through interface dispatch and whatnot.

And that's only if you want a reasonably accurate list. If you ever stop searching, you'll get an innacurate list. And now there's a problem that you're filtering the list down and leaving out real exceptions that can get thrown.

:-/

[Thaina]

@CyrusNajmabadi No man NO. You are still over thinking

What I said analyze metadata
is
analyze metadata of direct instance function and static function

So it must read metadata of Except because it is actually static function, just in another dll, which is obviously static syntax tree that could be analyzed, nothing relate to virtual or interface at all

I don't understand why you are too attached to virtual and interface while I was state it so clear that only direct instance function and static function

Is Enumerable.Except are virtual method? No it is just static extension method

[CyrusNajmabadi]

No it is just static extension method

It's a static extension method that may call other code taht you would then need to analyze including performing virtual dispatch.

Note: this is not hypothetical. That's exactly how the Linq operators work. You pass them an IEnumerable. So one of the very first things they do is call virtual methods on that IEnumerable they were given (that's how they get GetEnumerator in the first place).

[CyrusNajmabadi]

which is obviously static syntax tree that could be analyzed

There are no 'syntax trees' for metadata. There is metadata signatures and IL. The latter of which we don't even have an API in roslyn to access in order to do the analysis.

[CyrusNajmabadi]

I don't understand why you are too attached to virtual and interface

The reason i care about this is because we cannot give a good experience unless we do full analysis of the code that you're calling. Otherwise we will over-filter leading to correct and appropriate exceptions not being shown.

In order to full analysis, we must figure out what is going on with interfaces and whatnot. Consider your Except example. And say we have the following code:

c# void Foo() { try { x.Except(SomeMethod); // note: SomeMethod is not called, it is passed. } catch ( //<-- what do we show here? }

We'll need to figure out what 'Except' actually does internally. Internally it calls many virtual methods on the IEnumerable it is passed and it calls out to arbitrary 'Func' instances. We would then have to realize that that means that 'Except' could throw the exceptions that SomeMethod might throw. That analysis alone is already super difficult. At every step you have to consider what else might happen. If you don't then you'll end up with an inaccurate list. And we do not like inaccurate intellisense.

[CyrusNajmabadi]

@Thaina I highly encourage you to actually try doing this so that you can see what the problems would be. If you can actually solve this, and if you can do so in a manner that is fast enough for completion (whihc has a budget of around 10ms), then we would take the PR :)

[Thaina]

@CyrusNajmabadi

It's a static extension method that may call other code

What I care is only static and if other code is interface then just skip it. There could be a bunch of static code still analyzable deep down and more exception to list out

interface function also might throw or not throw any exception we could not know in advance anyway. This feature I propose just help us glean what we could be sure about it

There is metadata signatures and IL

I remember we have IL analysis and could analyze type signature. I don't know it not included in roslyn

[CyrusNajmabadi]

What I care is only static and if other code is interface then just skip it.

Then the list will be overfiltered, which will lead to a poor intellisense experience for users. That's not acceptable. We're not going to degrade things in such a common scenario.

[jmarolf]

Sounds like the following (big) workitems would need to be done in order for this to work well

1. Flow analysis API that could actually answer questions like this
2. A Roslyn API that can read metadata and return information to the flow analysis API
3. Some sort of Workspace level analysis engine that allows us to analyze the entire solution

These three pieces may exist someday, but today I think this would take a lot of work upfront and not be very accurate on the other end.

[Thaina]

@CyrusNajmabadi I think it much more better than currently we need to catch unknown exception. We don't know anything about what third party API could throw and it then hidden until the condition match that exception by accident to crash our system

I think at least exception that surely possible to be thrown should be known before hand

[CyrusNajmabadi]

@Thaina I'm not sure what you're asking for. Can you define what you'd like the experience to be? Right now it sounds like you're suggesting a feature that would actively degrade the experience in most cases (including the examples you provided).

The example i gave also showed how the experience would be degraded. These are simple cases and things would be made worse for developers.

[Thaina]

@CyrusNajmabadi But well, thanks for you remind me many times about interface and virtual function

Now I think it actually possible, to really overfiltering all possible class derived from that interface and all overridden function from all class included in compilation from all dll when we build a project

That would really give all possible exception that we could found on one try block

[CyrusNajmabadi]

@Thaina

1) Take a look at the example i provided. The exceptions can also come from any delegate invocations.
2) This analysis will be expensive. Remember that we have a 10ms budget here for computation.

[Thaina]

@CyrusNajmabadi OK to simplify it

Actually I was frustrate from exception thrown out of easy function like int.Parse

How many exception can it throw? If I want to carefully handle all the case and log it accurately and response to each kind of exception differently what should I do?

I think function like int.Parse is statically contain static logic to throw exception in listable set. When I write catch I should see all exception I could catch from int.Parse on top of exception list and I could correctly using namespace for that exception. That's the purpose

[Thaina]

@CyrusNajmabadi Well, sure if we write API we could not read anything from outside delegate that could throw anything in the future

But that's responsible for extender to catch their own exception. We just try to catch exception we could know about. We don't need to care about exception in the future

This analysis will be expensive

The analysis result could be cached on compilation or when we import reference dll

[CyrusNajmabadi]

Can you explain what benefit you see from this analysis versus just examining the tags that already exist? It seems like that would be vastly less costly and would still provide a good enough experience given we're not going to look into any form of indirection at all (And we can't look into IL).

[Thaina]

@CyrusNajmabadi Because tags is not always exist. Most of the time they don't write it. And we don't know about deeper function

```C#
// this might be some 3rd party API
int X(string text)
{
return int.Parse();
}

//
var x = X(); // Do we know X can throw exception of int.Parse ?
```

There are many factor that we can't relied on documentation comment

[Pilchie]

I don't think this is a feature that we would build into roslyn itself as stated.

What I could imagine however is a set of tags related to the xml doc comments themselves - reporting issues if you call a method that is documented to throw and you don't either catch or document that exception type yourself. Similarly if you override/implement but throw exceptions outside what the thing you are overriding is documented to throw.

[Pilchie]

With that said, someone could work on building this outside of the roslyn project itself using the APIs that we provide