Roslyn: Why C# compiler accepts and compiles so obvious error?

[ddur]

Version Used: C# 6

Steps to Reproduce:

1. AnyType Identifier { get { return Identifier; } }
2. AnyType Identifier => Identifier;

Expected Behavior: Fail to compile

Actual Behavior: Compiles, at run-time runs into infinite loop until Stack Overflow

[HaloFour]

That's perfectly legal syntax. The fact that you have an unbounded and uncontrolled recursion issue is a logical one. (To note, I do think that a compiler warning or a default analyzer warning would be useful. Perhaps a compiler error in so obvious a case, but new errors/warnings would probably only be considered in strict mode.)

See #7695

[vladd]

Detection of this particular case of infinite recursion doesn't bring a lot and is perhaps not worth the effort. (One will immediately see the problem after the first run.) Detection of an infinite recursion in general is impossible as it's the same as Halting Problem. There are several articles written by Eric Lippert on issues like this: 1, 2, 3.

[ddur]

I know that detecting infinite recursion is not always possible.
This is simple case and should be easily detected.

I'm working on small embedded DSL, defined by properties.
It is possible to detect loops in DSL by analyzing expression tree.
But if user defines Expression<Func<T>> expr => expr; there is nothing I can do.
Loop will be eventually coded by intent, by mistyping or by "Murphy law".

This is definitely compiler task.

Thanks @Pilchie

[ddur]

https://en.wikipedia.org/wiki/Microsoft_Terminator
http://mmjb.github.io/T2/

[gafter]

I'm treating this as a feature request for a new warning.

@ddur When you say "this is definitely a compiler task", are you ruling out it being implemented by a (non-compiler) analyzer?

[ddur]

Thanks @gafter
No, I expect to be at least warned.
I'm ruling out that error is ignored, either by compiler or analyzer.
This is not only endless loop. It contains unreachable (final) return statement.
All potential code after recursive call is unreachable.
If there is a code before recursive call that contains conditional return statement, then, I guess, deciding if this is an error or not can be non trivial case for analysis.

[ddur]

Hi @gafter
I'm glad you added Area-Analyzers but why removing Area-Compilers?
Why you refuse or do not want compiler improved?
Is it because of competition with another team or company?

[gafter]

The compiler's main job is to implement the language specification. The Analyzers area covers proposed new warnings that are not required by the language specification, whether implemented in the compiler or elsewhere.

[AlgorithmsAreCool]

Since I just got bit by this class of bug again, I want to bump this issue. I cannot imagine that self-referencing property are a common pattern and they are definitely a source of subtle and severe bugs that can bring down a process.

A warning for recursive getters would really be helpful here.
An if the getter does nothing else but return itself, I believe that should be an error

[CyrusNajmabadi]

@AlgorithmsAreCool

A warning for recursive getters would really be helpful here.

Warnings can be added by writing an analyzer that looks for this sort of thing. That way this sort of thing can be added without breaking existing code that depends on legal behavior the language has allowed. If you'd like help writing an analyzer here, i can walk you through things.

[AlgorithmsAreCool]

I resolve myself to write an analyzer to fix this for myself. But honestly I was hoping the default compiler would add this so C# is rid of this plague of typos for good.

Level with me, is there any chance of roslyn internalizing this into a default warning? Or is the breaking change not worth it?

[CyrusNajmabadi]

Level with me, is there any chance of roslyn internalizing this into a default warning? Or is the breaking change not worth it?

@mavasani Would you be interested in porting this to roslyn-analyzers? @AlgorithmsAreCool would you be willing to contirbute this to dotnet/roslyn-analyzers?

[AlgorithmsAreCool]

Yeah, I'll step up and give it a shot.

[mavasani]

I think this is similar to https://github.com/dotnet/roslyn-analyzers/issues/1478. I know that @jamesqo was very passionate about this one...

[mavasani]

Ah seems like that issue was blocked as we did not have any custom dataflow analysis support, which is now added to that repo, but is still evolving. @AlgorithmsAreCool - you can start with implementing a non-DFA based approach and we would be happy to take a contribution and in future extend it to even perform flow analysis for further preciseness.

[AlgorithmsAreCool]

Alright, should I move to that issue or create a new one on roslyn-analysers?

[mavasani]

Yes, feel free to comment on the issue that you are working on it - also ping me in case you need any help in setting up yourself to start working on it in the repo.

[CyrusNajmabadi]

Note: i think we could start with something super trivial. Just catching a property that directly calls itself. Anything more complex than that can come later

[AlgorithmsAreCool]

Sounds fair