Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix incorrect type resolution around array property accesses #323

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

Cirras
Copy link
Collaborator

@Cirras Cirras commented Dec 6, 2024

#313 introduced a regression in type resolution for plain array properties.

We started erroneously calling addResolvedDeclaration() before resolving array properties, where before we would exit immediately and return to argument disambiguation on the array accessor.

The effect was a subtle breakage of the type resolution around array properties. Take, for example, the expression Foo.Bar[0], where Bar is an array property returning string. The old behavior would yield type string, while the new behavior would yield type Char due to the premature addResolvedDeclaration() call updating the currentType to string before the array accessor was processed.

@Cirras Cirras requested a review from fourls December 6, 2024 03:30
Copy link
Collaborator

@fourls fourls left a comment

Choose a reason for hiding this comment

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

Logic of the change looks good - the extra tests are a really good idea. Just one thought about the method layout.

@Cirras Cirras force-pushed the array_property_type_resolution branch from 14e88d5 to d29ce7e Compare December 6, 2024 05:29
@Cirras Cirras requested a review from fourls December 6, 2024 05:29
#313 introduced a regression in type resolution for plain array
properties.

We started erroneously calling `addResolvedDeclaration()` before
resolving array properties, where before we would exit immediately
and return to argument disambiguation on the array accessor.

The effect was a subtle breakage of the type resolution around array
properties. Take, for example, the expression `Foo.Bar[0]`, where `Bar`
is an array property returning `string`. The old behavior would yield
type `string`, while the new behavior would yield type `Char` due to the
premature `addResolvedDeclaration()` call updating the `currentType` to
`string` before the array accessor was processed.
@Cirras Cirras force-pushed the array_property_type_resolution branch from d29ce7e to 392c692 Compare December 6, 2024 05:35
Copy link
Collaborator

@fourls fourls 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.

@Cirras Cirras merged commit 48b50a6 into master Dec 6, 2024
5 checks passed
@Cirras Cirras deleted the array_property_type_resolution branch December 6, 2024 05:40
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants