roslyn: TryStatementSyntax's Catches property should return all catch blocks regardless of directives.

Version Used: version 1.3.2

Steps to Reproduce:

For the following sample try catch block:

            try
            {
                //load the account
                //check for access to method
                int i = 0;
                return;
            }
#if DEBUG
            catch (ApplicationException ex)
            {
                throw new ApplicationException("The internal exception message was: " + ex.Message + ".  The stack trace is: " + ex.StackTrace + ".");
            }
#else
            catch (Exception e)
            {
                string str = e.Message;
                throw new ApplicationException(str);
            }
#endif

The TryStatementSyntax object’s Catches property only returns only one block depends the value of “DEBUG”. In my scenario, I need all the catch blocks returned so I could instrument the complete code. After talking with Cyrus of Roslyn team, this is the current behavior and there is no work around. I have to parse the file myself with the different values for the preprocessor directive set, then stich together the disparate edits together.

I would like to argue that this is very undesired behavior, as my guess is many applications around Roslyn could like to instrument the complete source code regardless surrounding symbol values, let user write complex inelegant code to stich them back is way from optimal. Roslyn team should consider to support this in future version to make the scenario much easier to use.

About this issue

Original URL
State: closed
Created 8 years ago
Reactions: 2
Comments: 22 (12 by maintainers)

Most upvoted comments

As I understand it, the issue is that Roslyn does not parse contents of the #if branch that was not taken at all. In your case, if Roslyn did parse it, it would still produce a nice syntax tree, but it’s not guaranteed to. For example, consider code like this:

try
{
    // whatever
}
#if DEBUG
catch (ApplicationException ex)
#else
catch (Exception ex)
#endif
{
    throw new ApplicationException(ex.Message);
}

Or even:

try
{
    // whatever
}
catch (
#if DEBUG
ApplicationException
#else
Exception
#endif
ex)
{
    throw new ApplicationException(ex.Message);
}

What would you expect to get in those cases? How is the tree supposed to look like?

One approach to solving your case that I can see would be to remove (or comment out) the directives, perform your instrumentation and then add the directives back.

Another approach would be to perform the instrumentation once with DEBUG set and then once with it unset, taking care not to add instrumentation twice to catch blocks that are not inside #ifs.

svick on Nov 16, 2016

This is orthgonal to try/catches. This is asking for the Roslyn system to have some way to provide information about code that the language defines as having no meaning. It’s similar in ask to having Roslyn tell you what the following code means:

try 
{
}
// catch (IOException e) { ... }
catch (Exception e)
{
}

This is something that is a major ‘ask’ due to there being near infinite possibilities for what might appear within that #if block. For example, the #if bug might open a string or comment that then changes the meaning of the other parts of the block (including making the remainder of the preprocessor directives parse differently).

These themselves can have massive downstream effects. For example, consider just basic preprocessor stuff like:

#if DEBUG
class 
#else
struct
#endif
MyClass {

}

Not only is syntax affected here, but there would be massive semantic fallout from this.

As such, the recommendation for cases like this is: parse your code depending on the sets of options you care about, figure out the changes you would make for the different parse possibilities, and then merge those changes in together to get the final result.

You could wait for us to try to provide a generalized solution for this. But it’s going to be super low on our list, and likely would not be available for years 😕

CyrusNajmabadi on Nov 16, 2016