Skip to content

Fix Issue 22039 - ICE on infinite recursion in default parameter#14934

Merged
RazvanN7 merged 1 commit intodlang:stablefrom
RazvanN7:Issue_22039
Mar 2, 2023
Merged

Fix Issue 22039 - ICE on infinite recursion in default parameter#14934
RazvanN7 merged 1 commit intodlang:stablefrom
RazvanN7:Issue_22039

Conversation

@RazvanN7
Copy link
Contributor

@RazvanN7 RazvanN7 commented Mar 1, 2023

When functionParameters is called, a pointer to the type of the call expression is passed so that the type is set. This is done because inout needs to be matched to one of the mutable, const, immutable qualifiers. As a measure of precaution, the first thing functionParameters does is to set that pointer to point to a TypeError. I guess this is done to catch any error that falls between the cracks, however, the downside is that you might end up with errors in your AST for which you have not outputted an error message.

This mechanism interferes with how default arguments are processed in the provided bug report:

int func(int x = func()) { return x; }

When the type of function func is analyzed, it needs to check that the type of the default arg matches the type of the parameter. As a consequence it performs semantic analysis on the function call of func. Then it ends up calling functionParameters with the type of function func (int(int x = func())) and a pointer to the type of the call expression. The catch is that this pointer points to a part of the type of func, therefore, when the pointer is set to TypeError the type of func is automatically updated to contain an error that is unaccounted for. This messes up the analysis of the default arg inside functionParameters and leads to the error being caught somewhere down the road where the compiler has no idea where it comes from, hence the ICE.

To fix this, I am deleting the line that bluntly sets the call type to error and if a default argument is missing its type information I'm just performing semantic on it on the spot. This seems to fix the issue and correctly output the recursive evaluation error.

Anyway, I find that functionParameteres is a complex beast doing all kinds of stuff. In my opinion, the way inout is handled here and how it uglifies the code is another justification for deprecating inout.

Although this is a regression, I'm not targeting stable since it's possible that some erroneous cases that would have been caught by the older mechanism slip through. Let's see what the tester says.

@dlang-bot
Copy link
Contributor

dlang-bot commented Mar 1, 2023

Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
22039 regression ICE on infinite recursion in default parameter

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "stable + dmd#14934"

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ICEs should go to stable though

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Mar 1, 2023

I think this PR has a certain level of risk, therefore it should be in master for a while.

As per @ibuclaw 's suggestion, retarget to stable.

@RazvanN7 RazvanN7 changed the base branch from master to stable March 1, 2023 14:39
@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Mar 1, 2023

Documentation failure is unrelated.

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Mar 2, 2023

Stable seems to be broken for MacOS. I am seeing the failure on multiple PRs.

@RazvanN7 RazvanN7 merged commit b9a4b28 into dlang:stable Mar 2, 2023
@ibuclaw ibuclaw mentioned this pull request Mar 3, 2023
RazvanN7 added a commit that referenced this pull request Mar 3, 2023
* Fix Issue 22039 - ICE on infinite recursion in default parameter (#14934)

* remove IRState.falseBlock - stable version

---------

Co-authored-by: Razvan Nitu <razvan.nitu1305@gmail.com>
Co-authored-by: Walter Bright <walter@walterbright.com>
@ibuclaw ibuclaw mentioned this pull request Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants