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.

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

No load error on at macroexpand #37857

Closed
wants to merge 1 commit into from
Closed

No load error on at macroexpand #37857

wants to merge 1 commit into from

Conversation

bramtayl
Copy link
Contributor

@bramtayl bramtayl commented Oct 2, 2020

There's three reasons I think this would be a good idea:

  1. Semantic: code that is macroexpanded is not loaded, at least, it's certainly not evaluated.

  2. Not particularly useful: load errors contain a line number and module, but that's not particularly helpful for macroexpand, which is usually used interactively

  3. Convenience: not having to unwrap loaderrors to test macros is more convenient. I think a strong case for this can be made just by looking at the tests I've changed that no longer need to do this.

@bramtayl
Copy link
Contributor Author

bramtayl commented Oct 2, 2020

Also worth noting that it's my understanding that errors can be changed for minor versions

@bramtayl
Copy link
Contributor Author

bramtayl commented Oct 9, 2020

Bump?

1 similar comment
@bramtayl
Copy link
Contributor Author

Bump?

@bramtayl
Copy link
Contributor Author

bramtayl commented Nov 8, 2020

Bump??

@JeffBezanson
Copy link
Sponsor Member

I agree. Rebase this and we can merge it.

@c42f
Copy link
Member

c42f commented Nov 9, 2020

I agree with removing LoadError!

However, note that this will break package tests spuriously because packages commonly test their macros with code like

@test_throws LoadError macroexpand(some_module, :(some_ex))

In my earlier effort to remove LoadError more widely (see #31882) I had a solution to this by grandfathering in LoadError in @test_throws handling.

@bramtayl
Copy link
Contributor Author

bramtayl commented Nov 9, 2020

@c42f oh I didn't realize there was previous work on this. Do you think it's worth it to try to do a deprecation? Any comments on whether the previous implementation was better/worse? (I barely knew enough C to do this PR).

@c42f
Copy link
Member

c42f commented Nov 9, 2020

A few thoughts:

  • It's worth reading the discussion on that PR for some background. Jeff's comment at Soft deprecation of LoadError in runtime #31882 is relevant.
  • I probably tried to do too much in that PR which is why it got stuck. Especially all the changes I was trying to make to interpreter backtraces were quite involved. So you can ignore most of the C code there.

For this PR, I think it's very worth taking the relevant code from the first commit of #31882 (see 3ee9d8f). Especially the changes to the Base tests and to the Test stdlib in do_test_throws, etc. This will prevent a lot of needless breakage in packages which currently depend on LoadError being thrown.

@bramtayl
Copy link
Contributor Author

Ok, I've included the deprecation and rebased as #38379

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants