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

fix(rome_js_analyze): noRedeclare accepts index signatures #4519

Merged
merged 8 commits into from
Jun 12, 2023
Merged

fix(rome_js_analyze): noRedeclare accepts index signatures #4519

merged 8 commits into from
Jun 12, 2023

Conversation

unvalley
Copy link
Contributor

@unvalley unvalley commented May 20, 2023

Summary

Closes #4478 #4545

Test Plan

cargo test -p rome_js_analyze -- no_redeclare

Handle these test cases:

// valid
type ValidIndexSignatures = {
	a: {
		[index: string]: string;
	};
	b: {
		[index: string]: string;
	};
};

// valid
interface A { [index: string]: string; [index: number]: string; }

// invalid
interface A { [index: number]: string; [index: number]: string; }

// valid #4545
let a: { [key: string]: string };
let b: { [key: string]: string };

Changelog

  • The PR requires a changelog line

Documentation

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

@netlify
Copy link

netlify bot commented May 20, 2023

Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 029c4a2
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/6487086b1954790008f9cbe1
😎 Deploy Preview https://deploy-preview-4519--docs-rometools.netlify.app/playground
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot added A-Linter Area: linter A-Parser Area: parser labels May 20, 2023
@unvalley unvalley marked this pull request as draft May 20, 2023 18:01
@Vivalldi
Copy link
Contributor

Anything we can do to help get this out of draft?

@unvalley
Copy link
Contributor Author

@Vivalldi

I need to accept the case below while fixing the problem. current implementation is not enough.

type IndexSignatures = {
    [index: number]: string,
    [index: string]: string
}

@github-actions github-actions bot removed the A-Parser Area: parser label Jun 3, 2023
chore: CHANGELOG.md

chore: fix indefinite articles

wip

wip

chore: fix comment

test: update snapshot

chore: update the CHANGELOG.md
@unvalley unvalley marked this pull request as ready for review June 3, 2023 07:31
Comment on lines 192 to 206
fn are_same_type_members(
first: &TsIndexSignatureTypeMember,
second: &TsIndexSignatureTypeMember,
) -> Option<bool> {
let first_text_range = first
.parent::<TsTypeMemberList>()?
.syntax()
.text_trimmed_range();

let second_text_range = second
.parent::<TsTypeMemberList>()?
.syntax()
.text_trimmed_range();
Some(first_text_range == second_text_range)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to check if we have the same parent TsTypeMember.
Other than using text_range, is there any other way to check that they are defined with the same parent (same location)?

Copy link
Contributor

@denbezrukov denbezrukov Jun 12, 2023

Choose a reason for hiding this comment

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

Can we compare their parent nodes?

    let first_parent = first
        .parent::<TsTypeMemberList>()?;

    let second_parent = second
        .parent::<TsTypeMemberList>()?;
    Some(first_parent == second_parent)

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. We should be able to plainly compare two nodes using ==

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I refactored.

Copy link
Contributor

@ematipico ematipico 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 to me!

@unvalley unvalley merged commit 7d9320d into rome:main Jun 12, 2023
@unvalley unvalley deleted the fix-no-redeclare-for-index-signatures branch June 12, 2023 14:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 lint/suspicious/noRedeclare - Index signatures in same object marked as invalid
4 participants