-
Notifications
You must be signed in to change notification settings - Fork 58
Support CfxLua #338
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
Support CfxLua #338
Conversation
|
Some of the tests dont pass in the workflow, I'll look into it tmrw |
b1ac9c0 to
942e98d
Compare
|
Would be glad if someone can look over it. I tested the workflows locally with Docker, and it keeps changing up the bytes, line & character values in the AST. Somehow, they are not the same in the workflow environment as on my Windows machine. |
|
Test should run now |
JohnnyMorganz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! I had an initial look - I haven't looked deep into the parser / tokenizer changes yet, I will do this in a follow up
full-moon/src/ast/luau.rs
Outdated
| } | ||
| /* | ||
| CompoundOp and CompoundAssignment have been moved to `compound.rs´, since CfxLua makes use of them as well. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we re-export the types here? To reduce the breaking change on downstream users
full-moon/src/ast/compound.rs
Outdated
| SlashEqual(TokenReference), | ||
| CaretEqual(TokenReference), | ||
|
|
||
| // luau sepcific |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be gated via cfg(feature = "luau") flags if they are only available in luau?
Some with the below for CfxLua
full-moon/src/ast/compound.rs
Outdated
| LeftShift(TokenReference), | ||
| RightShift(TokenReference), | ||
| BitwiseAndAssignment(TokenReference), | ||
| BitwiseOrAssignment(TokenReference), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To align with the other options. We try to make the names represent the characters rather than the logic, since in future >>= could mean something other than right shift operator for a different Lua dialect
| LeftShift(TokenReference), | |
| RightShift(TokenReference), | |
| BitwiseAndAssignment(TokenReference), | |
| BitwiseOrAssignment(TokenReference), | |
| DoubleLessThanEqual(TokenReference), | |
| DoubleGreaterThanEqual(TokenReference), | |
| AmpersandEqual(TokenReference), | |
| PipeEqual(TokenReference), |
full-moon/src/ast/mod.rs
Outdated
| #[cfg(any(feature = "lua52", feature = "luajit"))] | ||
| pub mod lua52; | ||
| #[cfg(feature = "lua54")] | ||
| #[cfg(any(feature = "lua54", feature = "cfxlua"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since cfxlua already extends lua54, we don't need to change this config since lua54 should always be enabled
| #[cfg(any(feature = "lua54", feature = "cfxlua"))] | |
| #[cfg(feature = "lua54")] |
full-moon/src/ast/mod.rs
Outdated
| /// A goto statement, such as `goto label` | ||
| /// Only available when the "lua52" or "luajit" feature flag is enabled. | ||
| #[cfg(any(feature = "lua52", feature = "luajit"))] | ||
| #[cfg(any(feature = "lua52", feature = "luajit", feature = "cfxlua"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above and in the other cases, I think this can be left alone
| #[cfg(any(feature = "lua52", feature = "luajit", feature = "cfxlua"))] | |
| #[cfg(any(feature = "lua52", feature = "luajit"))] |
full-moon/src/visitors.rs
Outdated
| visit_attribute => Attribute, | ||
| } | ||
|
|
||
| #[cfg(all(feature = "cfxlua", not(feature = "luau")))] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand this condition.
We can remove the visit_compound_assignment from the Luau section above and then use any
| #[cfg(all(feature = "cfxlua", not(feature = "luau")))] { | |
| #[cfg(any(feature = "cfxlua", feature = "luau"))] { |
full-moon/src/visitors.rs
Outdated
| visit_number, | ||
| visit_single_line_comment, | ||
| visit_string_literal, | ||
| visit_c_style_comment, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be gated by a cfxlua check
full-moon/src/visitors.rs
Outdated
| #[cfg(any(feature = "lua52", feature = "luajit"))] | ||
| use crate::ast::lua52::*; | ||
| #[cfg(feature = "lua54")] | ||
| #[cfg(any(feature = "lua54", feature = "cfxlua"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #[cfg(any(feature = "lua54", feature = "cfxlua"))] | |
| #[cfg(feature = "lua54")] |
full-moon/src/ast/mod.rs
Outdated
| /// A set constructor field, such as .a inside { .a } which is equivalent to { a = true } | ||
| #[display("{dot}{name}")] | ||
| #[cfg(feature = "cfxlua")] | ||
| SetConstructorField { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are already in Field, we don't need Field at the end of the name
| SetConstructorField { | |
| SetConstructor { |
full-moon/src/ast/parsers.rs
Outdated
| }; | ||
|
|
||
| #[cfg(any(feature = "cfxlua", feature = "luau"))] | ||
| use super::Var; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this was introduced by your IDE.
You should reference it through ast::Var instead, which is already exported
Resolve flaws with CFXLua feature
|
Hi @JohnnyMorganz, I’ve just implemented the suggested changes last night and verified that Clippy is now passing too. Let me know if there’s anything else you’d like to see. |
| PlusEqual(TokenReference), | ||
| MinusEqual(TokenReference), | ||
| StarEqual(TokenReference), | ||
| SlashEqual(TokenReference), | ||
| CaretEqual(TokenReference), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO for this and the checks you should do #[cfg(any(feature = "luau", feature = "cfxlua"))]--it makes it clear when adding more what's actually supported by Lua itself (nothing) and what isn't.
full-moon/src/ast/luau.rs
Outdated
| #[cfg(not(feature = "luau"))] | ||
| #[cfg_attr(docsrs, doc(cfg(not(feature = "luau"))))] | ||
| #[deprecated( | ||
| since = "full-moon 0.18.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're way past this already, IMO I would ditch since entirely since it doesn't add much value
full-moon/src/tokenizer/lexer.rs
Outdated
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Remove these
full-moon/src/util.rs
Outdated
| // If the vector length is 0 or there is at least one Some -> false | ||
| #[cfg(feature = "luau")] | ||
| #[allow(clippy::ptr_arg)] | ||
| pub fn no_luau_usage<T>(vec: &Vec<Option<T>>) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a good name--vec_empty_or_all_none?
resolve latest comments from @Kampfkarren
cfxlua+=, -=, *=, /=, <<=, >>=, &=, |=, and ^=t?.x?.y == nillocal a, b, c = tis equivalent tolocal a, b, c = t.a, t.b, t.ct = { .x, .y }is equivalent tot = { x = true, y = true }/* comment */`Hello, World!` -> 1395890823