-
-
Notifications
You must be signed in to change notification settings - Fork 661
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
The macro-in-macro check seems insufficient. #11403
Comments
This was an intentional change, the macro run-time now throws an exception if it encounters a macro call, and the typer doesn"t care about it otherwise. But yes, the way this errors isn"t ideal in this situation. It would be better if any function that has a macro call in macro context would cancel its typing, and then emit a useful error if it ends up being called. |
Although that might be a bit too simplistic too. I see no reason why code like this shouldn"t work: class Main {
static function main() {}
static function test(b:Bool) {
if (b) {
getValues();
}
}
public static macro function getValues() {
test(false);
return macro [];
}
} For this particular issue, the solution might be to support There might be other cases where the inserted |
Thanks for the fix. But I wonder if this is the right approach, because the underlying issue can take more than one form (I suspect there are quite a few): class Test {
static function main() {
switch getValues() {
case [1]: // Unrecognized pattern: [1]
default:
}
}
public static macro function getValues() {
return macro [1];
}
}
I really don"t see why it should. What is the use case here? Can there even be a use case for allowing code that MUST NOT run? My original point was that it indeed shouldn"t work. The only reliable effect of allowing this is to frustrate users. FWIW I really do think that when the typer sees a macro call in macro context it should error right there, much like it does with |
Failing during typing is not practical, but perhaps it"s indeed better to just fail if we"re calling a function that has a macro in macro context. I"m not sure though because any problem we run into here can also be reproduced by manually using |
I"m sort of worried by the whole approach, because it allows amassing piles of invalid code. This compiles with the latest change: class Test {
static function main() {
for (v in getValues()) {
#if macro
blarghfnth;// fnghhhtwrrt?
#end
}
}
public static macro function getValues() {
return macro [1];
}
} Allowing unrunnable code to compile in macro context just because it happens not to run feels like embedding the worst possible subset of PHP straight into the compiler. The only thing that can reliably produce is frustration. Moreover, there are cases that I"m not sure you can sensibly deal with:
Are you saying it is not practical now? Or ever? Because it it"s the latter, I really just don"t get why. Nothing good ever came from target context code unintentionally spilling into macro context. Even if it happens to compile at the time of writing, it"s because the planetary alignment was just right. At best, it adds build time. At worst it produces giant piles of errors after a subsequent innocuous change. I don"t think it"s doing anyone a favor to actually support this. Yes, failing during typing will probably mean that some code starts to break with "macro in macro" errors. If these get enough context, the code should be trivial to fix and will be better as a result. |
I mean not practical ever. Even the very first macro example in the manual would no longer compile and require I guess not typing parts of the function at all wasn"t a good idea. I"ll see how we can get better failures for some of these more arcane situations. |
Original discussion about disallowing macro in macro: #7348
It seems the check only occurs when actually calling a macro from a macro:
But not when calling a macro in macro context:
Does that really make sense?
The text was updated successfully, but these errors were encountered: