feat(rome_js_formatter): add format element label #2783

denbezrukov · 2022-06-24T16:49:29Z

Summary

This PR introduces the new IR element Label. This IR matches Prettier's label command.

https://github.com/prettier/prettier/blob/main/commands.md#label

This IR can be useful if representation depends on different representation of child content.
E.g., to decide how to print an assignment expression, we might want to know whether its right-hand side has been printed as a method call chain, not as a plain function call.

Test Plan

Added a new doc showing how to use the new IR

…ent-label # Conflicts: # crates/rome_formatter/src/format_element.rs

MichaReiser

Looks good

crates/rome_formatter/src/builders.rs

denbezrukov · 2022-06-24T20:48:49Z

@MichaReiser Could you help please?
There is an assertion static_assert!(std::mem::size_of::<crate::FormatElement>() == 32usize); for

pub enum FormatElement {
  ...
  Label(Label)
  ..
}
pub struct Label {
    pub(crate) content: Box<[FormatElement]>,
    label: &'static str,
}

May be we can:

use Label(Box<Label>) instead Label(Label).
use

pub struct Label {
    pub(crate) content: Box<[FormatElement]>,
    label: Box<&'static str>,
}

use another type for label? u16?

Is it ok to use &'static str type? Maybe there are some cases then need String? E.g. create dynamic label?

MichaReiser · 2022-06-24T21:08:59Z

I don't think using dynamic strings is a good idea as these labels drive the formatting logic. Therefore, using a static string sounds good, in which case the size is 24bytes, and 32 for FormatElement. Or is the assertion triggering with a static string?

An alternative is to do something similar to GroupId where it is a wrapper for a usize but holds a name in debug builds. But I don't think we need this optimisation just yet

MichaReiser · 2022-06-24T21:29:34Z

Or you could try to make Label a thin wrapper around TypeId. That has the nice added benefit that the compiler assigns compile time constants for each label. It also enforces constants (types actually) for each label, and comparing the labels in release is simply comparing two u64

This requires that Each label has its own Type (zero type struct or enum).

It would probably make sense to add a debug only name field, which you can automatically derive by using type_name

#[derive(Eq, PartialEq, Copy, Clone)]
pub struct Label {
  id: TypeId,
  #[cfg(debug_assertions)]
  label: &'static str
}

impl Label { 
  pub const fn of<T>() -> Self {
    Self {
      id: TypeId::of<T>(),
      #[cfg(debug_assertions)]
      label: type_name::<T>()
    }
  }
}

enum MemberNameLabel{}

let member_name = Label::of<MemberNameLabel>()

Argh, I've no idea how to create a code block on the iPhone 😅

denbezrukov · 2022-06-25T18:42:11Z

crates/rome_formatter/src/format_element.rs

+}
+
+impl LabelId {
+    pub fn of<T: ?Sized + 'static>() -> Self {


It's unstable api to make this function const.

Tracking Issue for const fn type_id rust-lang/rust#77125

denbezrukov · 2022-06-25T18:44:10Z

crates/rome_formatter/src/format_element.rs

+impl Debug for Label {
+    fn fmt(&self, fmt: &mut Formatter) -> fmt::Result {
+        fmt.debug_struct("")
+            .field("label_id", &self.label_id)


It looks:

label_id: LabelId { id: TypeId { t: 4451653586406524753, }, label: "rome_formatter::arguments::tests::test_nesting::SomeChain", },

denbezrukov · 2022-06-25T18:55:40Z

Or is the assertion triggering with a static string?

Yes, it is. I had two solutions how to handle this.

use Label(Box<Label>) as a variant for FormatElement.
use Box<&'static str> as a field for label.

Or you could try to make Label a thin wrapper around TypeId.

This way is great 😻 Thank you! I've edited the example of using this new IR.

https://github.com/rome/tools/pull/2783/files#diff-ef3073c2b45df5f202b850b362d40cb413b493036a5f726cb172fe25cf56645eR533-R569

Only one problem that we can't use const in pub const fn of<T>() -> Self. Because this API is unstable. I attached the link in the comment.

crates/rome_formatter/src/builders.rs

ematipico · 2022-06-27T08:47:38Z

crates/rome_formatter/src/builders.rs

+///     let is_labelled = match labelled_content {
+///         FormatElement::Label(labelled) => labelled.label_id() == label_id,
+///         _ => false,
+///     };


I am not sure this is the correct usage. This example shows that the consumer is forced to extract manually the FormatElement which is a low-level API for our formatter.

I wonder instead if we should provide an API, something like f.label_assert_of/label_id.assert_of (it asserts if the type of the label), which returns a boolean for us.

The suggestion is vague because I still need to understand how we do want to use the label and it can be used inside a real world example.

I've noticed this prettier API then I looked into isPoorlyBreakableMemberOrCallChain for variable assignment.

https://github.com/prettier/prettier/blob/9dd761a6e491ffff3856eea47fb10b4573b351a6/src/language-js/print/assignment.js#L329

They use label to understand that CallExpression has been printed as a member-chain.
I've just realized that it seems they traverse tree to extract label. Because they call isPoorlyBreakableMemberOrCallChain with properties array.

https://github.com/prettier/prettier/blob/9dd761a6e491ffff3856eea47fb10b4573b351a6/src/language-js/print/assignment.js#L216

But current this PR API allows look only last FormatElement.

We would need to come up with a different solution here, I suppose. I am not sure if printCallExpression actually prints in their main buffer or not (it seems not, which works for their logic). Regardless, they are able to extract the IR for they right-hand side of the assignment like expression and then decide the layout.

Our formatter works left-to-right, which means that once we write the right-hand side, it's there, unless we write it in a temporary buffer (which can be expensive, so we should avoid it).

I would suggest another solution for the assignment case. They create the label only when they actually create a member chain. They don't create the member chain in a specific case: https://github.com/prettier/prettier/blob/9dd761a6e491ffff3856eea47fb10b4573b351a6/src/language-js/print/member-chain.js#L342-L347

That condition is this one: https://github.com/rome/tools/blob/main/crates/rome_js_formatter/src/utils/member_chain/groups.rs#L42-L48

If we are able to use that logic inside the assignment like formatting, we might be able to not use the label.

Otherwise, the only solution that I can see is the write the right-hand side inside a temporary buffer, than inspect that buffer and check the label. But as said before, this has a big impact on memory usage.

Make sense👍
I guess that we can try it (:

What do you think about another case?
Now we have should_not_indent_if_parent_indents in binary_like_expression module. This function is aware about place where it will be printed. It uses should_break_after_operator from assignment_like module because when binary_like_expression is right part and layout is BreakAfterOperator assignment_like adds indent and to avoid double indent we have to check the same logic in binary_like_expression . May be we can invert this dependency and use label for binary_like_expression that it already has indent and check this label in assignment_like module.

tools/crates/rome_js_formatter/src/utils/binary_like_expression.rs

Lines 263 to 284 in 58c297f

fn should_not_indent_if_parent_indents(current_node: &JsAnyBinaryLikeLeftExpression) -> bool {

let parent = current_node.syntax().parent();

let parent_kind = parent.as_ref().map(|node| node.kind());

let great_parent = parent.and_then(|parent| parent.parent());

let great_parent_kind = great_parent.map(|node| node.kind());

match (parent_kind, great_parent_kind) {

(Some(JsSyntaxKind::JS_PROPERTY_OBJECT_MEMBER), _)

| (Some(JsSyntaxKind::JS_INITIALIZER_CLAUSE), Some(JsSyntaxKind::JS_VARIABLE_DECLARATOR)) => {

current_node

.as_expression()

.and_then(|expression| should_break_after_operator(expression).ok())

.unwrap_or(false)

}

(

Some(JsSyntaxKind::JS_RETURN_STATEMENT | JsSyntaxKind::JS_ARROW_FUNCTION_EXPRESSION),

_,

) => true,

_ => false,

}

}

I am not sure if that would work. Mainly because at the end of the format phase, where that function is used, the logic might add parenthesis to the binary expression, and the indentation has to stay inside the parenthesis. That's why checking the AST is better in this case.

Got it! Thank you!
So to emulate prettier label functionality we can add new extension inspect_label, InspectLabelBuffer and method for FormatElement has_label which traverses IR tree and search expected label?

EDIT:
It seems that second case also doesn't write in the main buffer.

tools/crates/rome_formatter/src/format_extensions.rs

Lines 217 to 227 in c518066

pub fn inspect(&mut self, f: &mut Formatter<Context>) -> FormatResult<&FormatElement> {

let result = self

.memory

.get_mut()

.get_or_insert_with(|| f.intern(&self.inner));

match result.as_ref() {

Ok(content) => Ok(content.deref()),

Err(error) => Err(*error),

}

}

tools/crates/rome_formatter/src/formatter.rs

Lines 166 to 173 in c518066

/// Formats `content` into an interned element without writing it to the formatter's buffer.

pub fn intern(&mut self, content: &dyn Format<Context>) -> FormatResult<Interned> {

let mut buffer = VecBuffer::new(self.state_mut());

crate::write!(&mut buffer, [content])?;

Ok(buffer.into_element().intern())

}

has_label which traverses IR tree and search expected label?

Is this what Prettier does? Does it traverse ALL the IR in order to find a label? I thought it just checks the first element

has_label which traverses IR tree and search expected label?

Is this what Prettier does? Does it traverse ALL the IR in order to find a label? I thought it just checks the first element

Sorry, you're right.
I double checked and prettier checks the first element.

I would suggest another solution for the assignment case. They create the label only when they actually create a member chain. They don't create the member chain in a specific case: https://github.com/prettier/prettier/blob/9dd761a6e491ffff3856eea47fb10b4573b351a6/src/language-js/print/member-chain.js#L342-L347

That condition is this one: https://github.com/rome/tools/blob/main/crates/rome_js_formatter/src/utils/member_chain/groups.rs#L42-L48

Could you please help me?🙏🏽 I can't find this case in Rome code:

https://github.com/prettier/prettier/blob/9dd761a6e491ffff3856eea47fb10b4573b351a6/src/language-js/print/member-chain.js#L342-L347

Because it seems this conditional:

https://github.com/rome/tools/blob/main/crates/rome_js_formatter/src/utils/member_chain/groups.rs#L42-L4

is prettier:

https://github.com/prettier/prettier/blob/a043ac0d733c4d53f980aa73807a63fc914f23bd/src/language-js/print/member-chain.js#L301-L304

UPDATE:
Do I understand correctly that prettier uses only one array for all groups and Rome uses two structs (HeadGroup and Groups)? I was wondering about cutoff value. It can be 2 and 3 depends on should_merge.
It seems that it always should be 1. Because Rome uses two structs and then should_merge is true it mutates Groups vec.

The shouldMerge value is used to essentially decide the layout of the formatting, and this affects the head of the group. So I went for a different approach because prettier's one was not working for us.

Yes, in theory cutoff should not be needed anymore, now that we actually mutate the groups vector

…ent-label

crates/rome_formatter/src/builders.rs

…ent-label

crates/rome_formatter/src/buffer.rs

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

denbezrukov · 2022-07-01T08:59:33Z

@ematipico Could I try to implement isPoorlyBreakableMemberOrCallChain after this PR?
https://github.com/prettier/prettier/blob/a043ac0d733c4d53f980aa73807a63fc914f23bd/src/language-js/print/assignment.js#L329

ematipico · 2022-07-01T09:06:25Z

@ematipico Could I try to implement isPoorlyBreakableMemberOrCallChain after this PR?
https://github.com/prettier/prettier/blob/a043ac0d733c4d53f980aa73807a63fc914f23bd/src/language-js/print/assignment.js#L329

Sure go ahead. I think it's the last piece for completing assignments

denbezrukov · 2022-07-01T09:11:35Z

@ematipico Could I try to implement isPoorlyBreakableMemberOrCallChain after this PR?
https://github.com/prettier/prettier/blob/a043ac0d733c4d53f980aa73807a63fc914f23bd/src/language-js/print/assignment.js#L329

Sure go ahead. I think it's the last piece for completing assignments

🥳🥳

There is one more piece

https://github.com/prettier/prettier/blob/a043ac0d733c4d53f980aa73807a63fc914f23bd/src/language-js/print/assignment.js#L145

I guess that we can implement canBreak the same way as willBreak.

denbezrukov and others added 3 commits June 24, 2022 19:48

feat(rome_js_formatter): add format element label

ecf2789

Merge remote-tracking branch 'upstream/main' into feature/format-elem…

e2ac4e6

…ent-label # Conflicts: # crates/rome_formatter/src/format_element.rs

merge

7d06443

MichaReiser reviewed Jun 24, 2022

View reviewed changes

crates/rome_formatter/src/builders.rs Outdated Show resolved Hide resolved

crates/rome_formatter/src/builders.rs Outdated Show resolved Hide resolved

review

0dced8e

denbezrukov added 3 commits June 25, 2022 21:28

add LabelId

3de8d8f

add LabelId

4faf2c8

add debug assertion

99874e5

denbezrukov commented Jun 25, 2022

View reviewed changes

ematipico reviewed Jun 27, 2022

View reviewed changes

denbezrukov added 4 commits June 28, 2022 16:26

Merge remote-tracking branch 'upstream/main' into feature/format-elem…

670a4e7

…ent-label

Merge remote-tracking branch 'upstream/main' into feature/format-elem…

43122bb

…ent-label

implement inspect_is_labelled extension and IsLabelledBuffer

c887ee4

remove nested conditional

2dd6486

denbezrukov commented Jun 29, 2022

View reviewed changes

crates/rome_formatter/src/builders.rs Show resolved Hide resolved

Merge remote-tracking branch 'upstream/main' into feature/format-elem…

3f69092

…ent-label

ematipico approved these changes Jul 1, 2022

View reviewed changes

crates/rome_formatter/src/buffer.rs Outdated Show resolved Hide resolved

ematipico requested a review from leops July 1, 2022 08:10

Update crates/rome_formatter/src/buffer.rs

e600e49

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

leops approved these changes Jul 1, 2022

View reviewed changes

ematipico merged commit 73f9b7e into rome:main Jul 1, 2022

denbezrukov deleted the feature/format-element-label branch July 1, 2022 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rome_js_formatter): add format element label #2783

feat(rome_js_formatter): add format element label #2783

denbezrukov commented Jun 24, 2022

MichaReiser left a comment

denbezrukov commented Jun 24, 2022 •

edited

Loading

MichaReiser commented Jun 24, 2022

MichaReiser commented Jun 24, 2022 •

edited

Loading

denbezrukov Jun 25, 2022

denbezrukov Jun 25, 2022

denbezrukov commented Jun 25, 2022

ematipico Jun 27, 2022 •

edited

Loading

denbezrukov Jun 27, 2022 •

edited

Loading

ematipico Jun 27, 2022

denbezrukov Jun 27, 2022

ematipico Jun 28, 2022

denbezrukov Jun 29, 2022 •

edited

Loading

ematipico Jun 29, 2022

denbezrukov Jun 29, 2022

denbezrukov Jul 5, 2022 •

edited

Loading

ematipico Jul 6, 2022

denbezrukov commented Jul 1, 2022

ematipico commented Jul 1, 2022

denbezrukov commented Jul 1, 2022

	fn should_not_indent_if_parent_indents(current_node: &JsAnyBinaryLikeLeftExpression) -> bool {
	let parent = current_node.syntax().parent();
	let parent_kind = parent.as_ref().map(\|node\| node.kind());

	let great_parent = parent.and_then(\|parent\| parent.parent());
	let great_parent_kind = great_parent.map(\|node\| node.kind());

	match (parent_kind, great_parent_kind) {
	(Some(JsSyntaxKind::JS_PROPERTY_OBJECT_MEMBER), _)
	\| (Some(JsSyntaxKind::JS_INITIALIZER_CLAUSE), Some(JsSyntaxKind::JS_VARIABLE_DECLARATOR)) => {
	current_node
	.as_expression()
	.and_then(\|expression\| should_break_after_operator(expression).ok())
	.unwrap_or(false)
	}
	(
	Some(JsSyntaxKind::JS_RETURN_STATEMENT \| JsSyntaxKind::JS_ARROW_FUNCTION_EXPRESSION),
	_,
	) => true,
	_ => false,
	}
	}

	pub fn inspect(&mut self, f: &mut Formatter<Context>) -> FormatResult<&FormatElement> {
	let result = self
	.memory
	.get_mut()
	.get_or_insert_with(\|\| f.intern(&self.inner));

	match result.as_ref() {
	Ok(content) => Ok(content.deref()),
	Err(error) => Err(*error),
	}
	}

	/// Formats `content` into an interned element without writing it to the formatter's buffer.
	pub fn intern(&mut self, content: &dyn Format<Context>) -> FormatResult<Interned> {
	let mut buffer = VecBuffer::new(self.state_mut());

	crate::write!(&mut buffer, [content])?;

	Ok(buffer.into_element().intern())
	}

feat(rome_js_formatter): add format element label #2783

feat(rome_js_formatter): add format element label #2783

Conversation

denbezrukov commented Jun 24, 2022

Summary

Test Plan

MichaReiser left a comment

Choose a reason for hiding this comment

denbezrukov commented Jun 24, 2022 • edited Loading

MichaReiser commented Jun 24, 2022

MichaReiser commented Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denbezrukov commented Jun 25, 2022

ematipico Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

denbezrukov Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denbezrukov Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denbezrukov Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denbezrukov commented Jul 1, 2022

ematipico commented Jul 1, 2022

denbezrukov commented Jul 1, 2022

denbezrukov commented Jun 24, 2022 •

edited

Loading

MichaReiser commented Jun 24, 2022 •

edited

Loading

ematipico Jun 27, 2022 •

edited

Loading

denbezrukov Jun 27, 2022 •

edited

Loading

denbezrukov Jun 29, 2022 •

edited

Loading

denbezrukov Jul 5, 2022 •

edited

Loading