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

Add Revise tests #27

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

etpinard
Copy link
Contributor

Start with some basic Revise tests, that currently pass on v1.9.1.

using some temporary module gymnastics
inside a new `revise.jl` test suite.

test that modifs in `@qstruct` definitions
are correctly revised.
which currently leads to a
```
MethodError: Cannot `convert` an object of type Nothing to an object of type Symbol
```
during `Revise.revise()`
@etpinard
Copy link
Contributor Author

then b459c6a leads to

image

The culprit seems to be in 2ed4967. Reverting it makes the test/revise.jl suite pass.

@cstjean
Copy link
Owner

cstjean commented Dec 16, 2023

Thank you for the bug report! 121bab4 makes the revise tests pass, at the cost of breaking some other tests. Revise doesn't seem to like __source__ interpolation.

Is Revise really broken on your side? Maybe the Revise test failures are just an artifact of the way the revise tests are performed, and it's the tests themselves that need updating.

I appreciate the test suite, and it was quite useful to me. That said, it would be a bit sad to add this kind of maintenance burden on all packages that use macros, for what is arguably a Revise failure (heroic as Revise is).

I think once this is fixed, I'd merge the test suite, but wouldn't hesitate to drop it if it becomes a burden.

@etpinard
Copy link
Contributor Author

Maybe the Revise test failures are just an artifact of the way the revise tests are performed, and it's the tests themselves that need updating.

No, the tests are working as expected. We can replicate the

MethodError: Cannot `convert` an object of type Nothing to an object of type Symbol

errors in real life scenarios.

@etpinard
Copy link
Contributor Author

MacroTools.@q seems to be part of the problem.

With

diff --git a/test/assets/within-macro1.jl b/test/assets/within-macro1.jl
index 36e468b..f639abd 100644
--- a/test/assets/within-macro1.jl
+++ b/test/assets/within-macro1.jl
@@ -3,7 +3,7 @@ using MacroTools: @q, @capture

 macro mymodel(def)
     @capture(def, model_(; params__))
-    esc(@q begin
+    esc(quote
             $QuickTypes.@qstruct_fp $model(; $(params...))
         end)
 end
diff --git a/test/assets/within-macro2.jl b/test/assets/within-macro2.jl
index 76da376..d27b4a6 100644
--- a/test/assets/within-macro2.jl
+++ b/test/assets/within-macro2.jl
@@ -3,7 +3,7 @@ using MacroTools: @q, @capture

 macro mymodel(def)
     @capture(def, model_(; params__))
-    esc(@q begin
+    esc(quote
             $QuickTypes.@qstruct_fp $model(; $(params...))
         end)
 end

the Revise error goes away.

@cstjean
Copy link
Owner

cstjean commented Dec 19, 2023

Yes, @q strips away all line numbers, which Revise doesn't seem to like. I would try:

res = esc(@q begin
             $QuickTypes.@qstruct_fp $model(; $(params...))
         end)
res.args[1].args[2] = __source__
return res

The problem with normal quote is that the line numbers point to the macro definition code, which isn't great. Eg. before my latest PR, @qstruct Foo(); @which Foo() would point to the QuickTypes code, whereas we'd like it to point to the module where @qstruct Foo() is written. That's what the @q + res.args[1].args[2] = __source__ combo achieves.

Come to think of it, maybe the @q isn't necessary... But still, I don't think it hurts. quote puts a lot of LineNumberNode everywhere, and almost none of them are desirable from my POV.

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.

2 participants