Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Compiler crash with recursive local function parameter default value #16451

Closed
gafter opened this issue Jan 12, 2017 · 9 comments
Closed

Compiler crash with recursive local function parameter default value #16451

gafter opened this issue Jan 12, 2017 · 9 comments
Assignees
@agocke
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Bug New Language Feature - Local Functions Local Functions Urgency-Soon
Milestone
15.1

Comments

@gafter
Copy link
Member

gafter commented Jan 12, 2017

Typing the following code into a C# project causes VS to crash (stack overflow in the compiler).

class C
{
    public static void Main(int arg)
    {
        int Local(int x = Local()) => 2;
    }
}
@gafter gafter added this to the 2.0 (RTM) milestone Jan 12, 2017
@AdamSpeight2008
Copy link
Contributor

I'm surprise it even compiles, as the default value part of the optional parameter requires a compile-time constant.

@gafter
Copy link
Member Author

gafter commented Jan 12, 2017

@AdamSpeight2008 It doesn't compile without an error. The compiler crashes before it has a chance to produce the error.

@gafter
Copy link
Member Author

gafter commented Jan 12, 2017

See https://github.com/dotnet/roslyn/pull/16315/files#r95429886 for a conversation that may be related to this.

@gafter gafter mentioned this issue Jan 12, 2017
Closed
@AdamSpeight2008
Copy link
Contributor

@gafter I was saying that it shouldn't even compile. To me it sounds like it trying to see if the result of the default value is a constant, so it evaluates Local. And further down the rabbit hole we go.

@gafter
Copy link
Member Author

gafter commented Jan 13, 2017

@AdamSpeight2008 Agreed, it should not even compile. But the compiler should terminate nicely after printing a nice, friendly error message rather than crashing.

@AdamSpeight2008
Copy link
Contributor

@gafter So is failing to parse? or Bind?

@agocke
Copy link
Member

agocke commented Jan 14, 2017

@AdamSpeight2008 Binding is failing in non-terminating recursion.

In essence, when Local is being bound to a symbol, it forces its parameters to be created and bound, meaning the default parameter is bound, which happens to be Local(), which means Local must be bound...

@jcouv
Copy link
Member

jcouv commented Jan 19, 2017

Tracked by VSO https://devdiv.visualstudio.com/DevDiv/_workitems?id=369812&_a=edit

@jaredpar jaredpar assigned AlekseyTs and unassigned agocke Jan 24, 2017
@agocke agocke assigned agocke and unassigned AlekseyTs Jan 25, 2017
@agocke agocke mentioned this issue Jan 25, 2017
Closed
agocke added a commit to agocke/roslyn that referenced this issue Jan 26, 2017
@agocke
Make local function parameters lazier
dfc6aba 
Local function parameters seemed to be designed to bind their
parameters' default values eagerly, as opposed to methods which bind
their default values lazily. This creates the possibility of infinite
binder recursion if the default value mentions the local function
itself (which has yet to be fully bound).

Fixes dotnet#16451
@jaredpar jaredpar modified the milestones: 2.1, 2.0 (RTM) Jan 26, 2017
@agocke agocke mentioned this issue Feb 8, 2017
Merged
@agocke agocke added the 4 - In Review A fix for the issue is submitted for review. label Feb 15, 2017
agocke added a commit to agocke/roslyn that referenced this issue Mar 6, 2017
@agocke
Make local function default parameter value binding lazy
3da2068 
Current strict binding can cause circularity problems when local
functions are referenced. This change causes local functions to use lazy
default parameter binding, similar to methods, and then forces their
construction when diagnostics are requested for the local function.

This also requires a mechanism for recording declaration diagnostics
outside of adding to the compilation's DeclarationDiagnostics. A new
type, DeclarationDiagnosticStore is introduced as an abstraction to
store declaration diagnostics on either the compilation or in a local
DiagnosticBag, depending on the needs of the symbol storing diagnostics.

Fixes dotnet#16451
agocke added a commit that referenced this issue Mar 7, 2017
@agocke
Make local function default parameter value binding lazy (#17014)
2b6d6f2 
Make local function default parameter value binding lazy

Current strict binding can cause circularity problems when local
functions are referenced. This change causes local functions to use lazy
default parameter binding, similar to methods, and then forces their
construction when diagnostics are requested for the local function.

This also requires a mechanism for recording declaration diagnostics
outside of adding to the compilation's DeclarationDiagnostics. A new
type, DeclarationDiagnosticStore is introduced as an abstraction to
store declaration diagnostics on either the compilation or in a local
DiagnosticBag, depending on the needs of the symbol storing diagnostics.

Fixes #16451, #17293
@khyperia
Copy link
Contributor

@agocke Randomly stumbled upon a skipped test due to this issue: LocalFunctionTests.RecursiveParameterDefault. Seems to be a dupe test of LocalFunctionTests.RecursiveDefaultParameter (which is not skipped), should probably remove it at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@agocke agocke

Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Bug New Language Feature - Local Functions Local Functions Urgency-Soon
Projects
None yet
Milestone
15.1
Development

Successfully merging a pull request may close this issue.

Make local function parameters bind lazily agocke/roslyn
7 participants
@jaredpar @agocke @AdamSpeight2008 @khyperia @gafter @AlekseyTs @jcouv