Skip to content

fix handling of static impure functions nested in pure functions (issues 20047 and 20050)#10172

Merged
thewilsonator merged 3 commits intodlang:masterfrom
aG0aep6G:static-nested-in-pure
Jul 15, 2019
Merged

fix handling of static impure functions nested in pure functions (issues 20047 and 20050)#10172
thewilsonator merged 3 commits intodlang:masterfrom
aG0aep6G:static-nested-in-pure

Conversation

@aG0aep6G
Copy link
Contributor

This is mostly deleting a bunch of code. As far as I can tell, it was all based on a wrong, overly complicated model of how things should work. But it's very possible that I'm missing crucial details.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @aG0aep6G! 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
20047 normal call of static nested function ignores purity
20050 normal pure function should be able to return function pointer to impure static nested function

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + dmd#10172"

@RazvanN7
Copy link
Contributor

I've tried to understand that code in the past and failed. It looks much more cleaner now.

Copy link
Contributor

Choose a reason for hiding this comment

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

please factor this out into a function, e.g:

if (!f.isPure() && checkPureCall(sc.func))
    return true;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please factor this out into a function

Done. I've called it checkImpure, though.

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.

Otherwise looks good.

@thewilsonator thewilsonator merged commit 22113af into dlang:master Jul 15, 2019
@aG0aep6G aG0aep6G deleted the static-nested-in-pure branch July 15, 2019 20:22

/*
Check if sc.func is impure or can be made impure.
Returns true on error, i.e. if sc.func is pure and cannot be made impure.
Copy link
Member

Choose a reason for hiding this comment

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

@aG0aep6G @thewilsonator when writing function comments, please use the Ddoc form.

Also, the function is not checking for an error, and it does more than check - it sets an impure state.

The phrase "cannot be made impure" makes little sense, as a pure function can always be called by an impure one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, the function is not checking for an error, and it does more than check - it sets an impure state.

How about:

    /*********************************************
     * Try marking sc.func as impure.
     * Returns true if sc.func is pure and cannot be made impure.
     */
    private static bool tryMakingImpure(Scope* sc) { ... }

The phrase "cannot be made impure" makes little sense, as a pure function can always be called by an impure one.

This is about the case where a (pure) function tries calling an impure one. sc.func is the calling function, and we try to mark it as impure. But that might fail (because of an explicit pure attribute). In that case, sc.func "cannot be made impure". Makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe this name and comment:

    /*********************************************
     * An impure function is called in sc. Propagate the impurity to the
     * surrounding function (sc.func).
     * Returns true if sc.func is pure and cannot be made impure.
     */
    private static bool propagateImpure(Scope* sc)

@PetarKirov
Copy link
Member

PetarKirov commented Jul 16, 2019

CC @tgehr @dnadlinger

@tgehr
Copy link
Contributor

tgehr commented Jul 16, 2019

Great! I've tried to do something like this before but didn't get the test suite to pass.

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.

7 participants

Comments