Misleading diagnostic for constant false when clause

[gafter]

Version Used: 15.3 (master)

Steps to Reproduce:

1. compile

            try
            {
            }
            catch (Exception) when (false)
            {
            }

Expected Behavior: Warning: filter expression is constant 'false'; consider removing try-catch.

Actual Behavior: Warning: filter expression is constant, consider removing the filter.

It would be a change of behavior to remove the filter as suggested by the diagnostic.

[elijah6]

Would it be okay for me to work on this?
I think I can supply the constant from boundFilter and pass it in as argument for Error(...) in Binder_Statements.cs where ErrorCode.WRN_FilterIsConstant is used, and check other usages such as tests. It seems reasonable in terms of complexity for me to start contributing :)

[elijah6]

I realised that bool.ToString() returns False or True instead of false or true. Should the warning message be consistent with the ToString() implementation (i.e. with Capital), or the keywords (i.e. lowercase)?
Since we can also do catch (Exception) when (default) in c# 7.1, I guess we are supposed to use the name that appears in the code (i.e. false, true, default).

[elijah6]

I misunderstood the requirement. It should also suggest different action based on the constant value..
But I think more clarification is required for the following cases;

• Should it suggest 'consider removing try-catch' when the whole try-catch statement is redundant (e.g. the example given above)?
  • Or should it suggest 'consider removing catch clause' only when the specific filter is false/default?
• How about the filter expression for switch statement? Should the warning be more general, or should there be separate warnings for each?

[gafter]

@elijah6 A fix for this would probably have to include code-gen changes. I believe we currently produce bad code for this (because the compiler notices that the catch body is unreachable). See #18678

[gafter]

We don't have warnings for the switch statement that are problematic, though they might be useful.

Yes, I think we'd need to distinguish in our diagnostic between just the catch clause being redundant, and the whole try statement being replaceable by the try block.

[gafter]

@elijah6 The diagnostic should use the C# name for the keyword true or false.

[elijah6]

Thank you @gafter . I will work on the warning part on this issue first and maybe look at the code gen part as well separately.

I am assuming that, in the following case, the warning will be filter expression is a constant 'false'. Consider removing the catch clause on each of the catch clauses. Only when there is a single catch clause with 'false' filter, it will say filter expression is a constant 'false'. Consider removing the try-catch block. This is so that we don't try to do too much (i.e. evaluate all catch filters and if they are all 'false', suggest removing try-catch block) and also so that each catch filter expression gets some warning at each level. Is that appropriate? Or should it actually evaluate all catch clauses and suggest removing the try-catch block on one of the catch clauses?

   try
   {
   }
   catch (SomeOtherException) when (false)  // filter expression is a constant 'false'. Consider removing the catch clause
   {
   }
   catch (Exception) when (false)  // filter expression is a constant 'false'. Consider removing the catch clause
   {
   }

   try
   {
   }
   catch (Exception) when (false)  // filter expression is a constant 'false'. Consider removing the try-catch block
   {
   }

[gafter]

Fixed in #18933