Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat: strip prefix from methods in code generation #1750

Merged
merged 6 commits into from
Nov 10, 2021

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Nov 5, 2021

Summary

Part of #1725

Closes #1733

This PR improves the codegen by forcing to use labels when writing ungrammar.

Test Plan

cargo test
cargo check
cargo lint

@@ -265,7 +265,7 @@ impl TsTypeAliasDecl {
pub fn type_token(&self) -> Option<SyntaxToken> { support::token(&self.syntax, T![type]) }
pub fn type_params(&self) -> Option<TsTypeParams> { support::child(&self.syntax) }
pub fn eq_token(&self) -> Option<SyntaxToken> { support::token(&self.syntax, T ! [=]) }
pub fn ts_type(&self) -> Option<TsType> { support::child(&self.syntax) }
pub fn ty(&self) -> Option<TsType> { support::child(&self.syntax) }
Copy link
Contributor

Choose a reason for hiding this comment

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

I hate that we can't have "type" as identifiers in Rust. There is one option: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=528a1f56f1209a1b4e5492ecec89c4ac

pub struct A {
    r#type: String
}

It is not great, but maybe it is something we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it work for methods? Because ty is a method in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does work but it also requires to change the call-sites to n.r#type() playground

We may be able to change the type name in the future. For example, is this a type reference or a type binding?

Copy link
Contributor Author

@ematipico ematipico Nov 8, 2021

Choose a reason for hiding this comment

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

Personally, I wouldn't call it type at all because it could create confusion depending on what we are evaluating.

For example:

function foo<T, S, V>(opts: Options): Foo {}

If we had a simple r#type() function, what's the type referred to? Foo or <T, S, V>? I'd prefer to have more descriptive names.

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

There might have been a misunderstanding in #1733 The title says that labels should be enforced but the content of the task sets the goal to remove the need for explicit labels:

Change the codegen to strip the Js extensions from field names to reduce the need for explicit labels. For example, ExpressionStatement must use an explicit label for its inner expression so that it isn't named js_expression.

I personally would find it valuable if I don't need to prefix all children because it makes the grammar super verbose.

Comment on lines 510 to 519
let mut final_name = name.clone();
for prefix in LANGUAGE_PREFIXES {
final_name = final_name.replace(prefix, "");
}
if final_name == "type" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we still need this if we use explicit labels anyway? Or is this for the node types?

Copy link
Contributor Author

@ematipico ematipico Nov 8, 2021

Choose a reason for hiding this comment

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

It's because type is a reserved keyword and we ca still do something like:

Foo = type: Type

Which would cause creating something like:

struct Foo {
	pub fn type() -> SyntaxResult<Type> {}
}

We could make it better, and throw an error during the code generation saying that type is denied as name for a label.

xtask/src/codegen/kinds_src.rs Outdated Show resolved Hide resolved
@ematipico
Copy link
Contributor Author

There might have been a misunderstanding in #1733 The title says that labels should be enforced but the content of the task sets the goal to remove the need for explicit labels:

Change the codegen to strip the Js extensions from field names to reduce the need for explicit labels. For example, ExpressionStatement must use an explicit label for its inner expression so that it isn't named js_expression.

I personally would find it valuable if I don't need to prefix all children because it makes the grammar super verbose.

I can close the PR if you want. Sorry for the misunderstanding.

@MichaReiser
Copy link
Contributor

There might have been a misunderstanding in #1733 The title says that labels should be enforced but the content of the task sets the goal to remove the need for explicit labels:

Change the codegen to strip the Js extensions from field names to reduce the need for explicit labels. For example, ExpressionStatement must use an explicit label for its inner expression so that it isn't named js_expression.

I personally would find it valuable if I don't need to prefix all children because it makes the grammar super verbose.

I can close the PR if you want. Sorry for the misunderstanding.

I do find it valuable that we automatically strip the Js prefix because it would then not be required to have explicit labels and I think your PR already accounts for that, doesn't it?

@ematipico
Copy link
Contributor Author

There might have been a misunderstanding in #1733 The title says that labels should be enforced but the content of the task sets the goal to remove the need for explicit labels:

Change the codegen to strip the Js extensions from field names to reduce the need for explicit labels. For example, ExpressionStatement must use an explicit label for its inner expression so that it isn't named js_expression.

I personally would find it valuable if I don't need to prefix all children because it makes the grammar super verbose.

I can close the PR if you want. Sorry for the misunderstanding.

I do find it valuable that we automatically strip the Js prefix because it would then not be required to have explicit labels and I think your PR already accounts for that, doesn't it?

I removed the logic that forces to use a label and kept the stripping part. Thank you for the suggestions!

@ematipico ematipico changed the title feat: force nodes to have labels feat: strip prefix from methods in code generation Nov 8, 2021
Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

And our code gen becomes better and better every day :)

Comment on lines 507 to 513
for prefix in LANGUAGE_PREFIXES {
final_name = String::from(
final_name
.strip_prefix(prefix)
.unwrap_or_else(|| final_name.as_str()),
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We may now end up stripping multiple prefixes (probably super rare). Might be safer to find the first prefix and then strip it Playground

 let prefixes = HashSet::from(["js", "ts", "tsx"]);
  let input = "js_expression";
  
  
  let (prefix, tail) = input.split_once('_').unwrap_or(("", input));
  
  let name = if prefixes.contains(prefix) {
      tail
  } else {
      input
  };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ultra rare but let's do it! The safer, the better

@ematipico ematipico merged commit 8f8edfd into rome:main Nov 10, 2021
@ematipico ematipico deleted the feature/forcel-label-codegen branch November 10, 2021 14:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strip Js from codegen and enforce usage of labels in codegen
3 participants