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

fix(rome_formatter): fix format range #4295

Merged
merged 3 commits into from
Mar 17, 2023

Conversation

denbezrukov
Copy link
Contributor

@denbezrukov denbezrukov commented Mar 11, 2023

Summary

  1. Fix format issues with incorrect replace rage in the source text.
  • crates/rome_js_formatter/tests/specs/js/module/range/range_parenthesis_after_semicol.js.snap
  • crates/rome_js_formatter/tests/specs/js/module/range/range_parenthesis_after_semicol_1.js.snap
  1. Introduce <<<ROME_RANGE_START>>> and <<<ROME_RANGE_END>>> for snapshot testing.
  2. Fix format the range issue if we get a token in the transformed tree.

UPD:

Let say we want to format with range range = 5..94 this code:

a?.b
const a = () => {sdasdsa;
	kek;
	dsad<<<ROME_RANGE_START>>>sa;
	123;
	("y̆es");
};
console.log("🚀".length);
		<<<ROME_RANGE_END>>>
("Jan 1, 2018 – Jan 1, 2019");

We need to calculate source and dest ranges.

  • source range we use to find a position in the source text where we need to replace the substring.
  • dest range we use to get the substring from the formatted code that we need to replace in the source text.

The example has a problem for the right end. We get the result:

a?.b
const a = () => {
	sdasdsa;
	kek;
	dsadsa;
	123;
	("y̆es");
};
console.log("🚀".length);

( <--- the problem
		
("Jan 1, 2018 – Jan 1, 2019");

it has the following source map:

   ...
    SourceMarker {
        source: 93,
        dest: 96,
    },
    SourceMarker {
        source: 94,
        dest: 97,
    },
    SourceMarker {
        source: 94,
        dest: 99,
    },
    SourceMarker {        <----- Now we take this range.
                                 Because we have a condition `marker.source <= prev_marker.source`
        source: 94,
        dest: 100,       <----- here
    },
    SourceMarker {   <---- But we need to take this one.
        source: 99,     
        dest: 100,       <----- and here
    },
    SourceMarker {
        source: 133,
        dest: 133,
    },
    ...

We can solve the issue by checking markers with the same dest but a greater source.

The same logic we can apply for a left side.

Test Plan

cargo test

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented Mar 11, 2023

Deploy Preview for docs-rometools canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 8407dd9
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/640d72c81194170007200611

@github-actions github-actions bot added the A-Formatter Area: formatter label Mar 11, 2023
@denbezrukov denbezrukov marked this pull request as draft March 12, 2023 13:15
@denbezrukov denbezrukov marked this pull request as ready for review March 12, 2023 13:38
@denbezrukov
Copy link
Contributor Author

Note:
Need to check in another PR:


a?.b
const a = () => {sdasdsa;
	kek;
	dsadsa;
	123;
	("y̆es");
};
console.log("🚀".length);
("Jan 1,<<<ROME_RANGE_START>>> 201<<<ROME_RANGE_END>>>8 – Jan 1, 2019");123

@MichaReiser
Copy link
Contributor

Could you extend the summary to explain what the root cause is? It would help me with reviewing the changes.

@MichaReiser MichaReiser self-requested a review March 13, 2023 08:53
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.

Looks good and thanks for fixing it ! Range formatting and source maps are 🤯

@denbezrukov denbezrukov merged commit 07488ee into rome:main Mar 17, 2023
@denbezrukov denbezrukov deleted the fix/format-range branch March 17, 2023 08:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Formatter Area: formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants