Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add typemeasure to literalexpr #547

Merged
merged 20 commits into from
Jul 26, 2023

Conversation

dawedawe
Copy link
Contributor

I think that's the PSI structure we talked about.
It's not explicitly using the range of < ... > but it seems to work by advansing the lexer to <

@auduchinok
Copy link
Collaborator

@dawedawe Yes, it's just as we've discussed. If FCS doesn't provide actual ranges for < then it probably should. It seems like it has ranges inside its cases, and it should be possible to add Range property. It might be useful for Fantomas as well.

The next step here would be to define grammar rules for the measure kinds:

typeMeasure:
  namedMeasure |
  productMeasure |
  ...

namedMeasure:
  typeUsage<TYPE_USAGE, TypeUsage>;

productMeasure:
  typeMeasure<MEASURE_1, Measure1>
  STAR
  typeMeasure<MEASURE_2, Measure2>;

...

After that you'll need to reflect that in the structure produced by the builder too.

Copy link
Collaborator

@auduchinok auduchinok left a comment

Choose a reason for hiding this comment

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

@dawedawe Everything looks much better now, thanks! I've also left some comments on the tree structure and tests, please take a look when you have time.

@dawedawe dawedawe force-pushed the add_typemeasure_to_literalexpr branch from 17a3428 to 1577b31 Compare July 25, 2023 13:23
@dawedawe dawedawe changed the base branch from net232 to net233 July 25, 2023 13:24
Copy link
Collaborator

@auduchinok auduchinok left a comment

Choose a reason for hiding this comment

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

@dawedawe This is very nice work, there's just a pair of small issues with the structure/grammar correspondence. I've also left few other comments, but they are not as important.

Comment on lines +12 to +13
INamedTypeUsage
ITypeReferenceName
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've just realized: perhaps a type usage wasn't needed here, and having just a type reference would be enough for features like Rename. We should probably try removing it when updating this code after FCS is updated.

ReSharper.FSharp/src/FSharp.Psi/src/FSharp.psi Outdated Show resolved Hide resolved
@dawedawe dawedawe marked this pull request as ready for review July 26, 2023 13:09
Copy link
Collaborator

@auduchinok auduchinok left a comment

Choose a reason for hiding this comment

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

@dawedawe Great work, thanks!

@auduchinok auduchinok merged commit e792dcd into JetBrains:net233 Jul 26, 2023
@dawedawe dawedawe deleted the add_typemeasure_to_literalexpr branch July 26, 2023 13:21
@dawedawe dawedawe restored the add_typemeasure_to_literalexpr branch July 26, 2023 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants