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 Delimiter tokens to the parser #3644

Closed
wants to merge 61 commits into from

Conversation

eureka-cpu
Copy link
Contributor

@eureka-cpu eureka-cpu commented Dec 21, 2022

Main issue is #5140
Adds Delimiter tokens, and updates their corresponding use cases.

Ref #3634 #3734
Closes #3635
Closes #3594

@eureka-cpu eureka-cpu added the bug Something isn't working label Dec 21, 2022
@eureka-cpu eureka-cpu self-assigned this Dec 21, 2022
@mohammadfawaz mohammadfawaz requested a review from a team December 21, 2022 21:15
@mohammadfawaz
Copy link
Contributor

Looks good! But you have some E2E test failures that need to be addressed. Specifically, test.toml for:

    should_fail/trait_method_signature_mismatch ... failed
    should_fail/abi_method_signature_mismatch ... failed

should be updated.

@mohammadfawaz
Copy link
Contributor

mohammadfawaz commented Jan 5, 2023

Ok so I will write my thoughts on the failing test here:
for trait_method_signature_mismatch, we used to emit the following error:
image
Now, we just emit this (not pointing to any location):
image
We previously used to point to the function span itself because the return type is missing (i.e. () implicitly). Now that we’re using Span::dummy(), the error is different and doesn’t point to anything which is not good. I thought we didn’t need this span for anything but clearly we do.
So I looked at what Rust does… it seems to point to the span of the following character as in:
image
So I suggest we do the same instead of returning a dummy span.

sdankel
sdankel previously approved these changes Jan 7, 2023
eureka-cpu added a commit that referenced this pull request Jan 10, 2023
Closes #3634 

Before: 
![Screen Shot 2023-01-09 at 7 26 05
PM](https://user-images.githubusercontent.com/57543709/211441217-c525c7d9-49db-40b9-8e74-2f21b3bd4c43.png)
After: 
![Screen Shot 2023-01-09 at 7 26 17
PM](https://user-images.githubusercontent.com/57543709/211441245-e9ea357b-e1c7-45de-9142-69712604b193.png)

This was originally to be closed in #3644 due to their similarity, but I
came to realize how we handle implicit returns for function signatures
will require some significant refactoring.
@eureka-cpu eureka-cpu changed the title Fix unit type spans Fix spans for functions that return implicitly Jan 10, 2023
@eureka-cpu eureka-cpu added big this task is hard and will take a while compiler: parser Everything to do with the parser labels Jan 10, 2023
sway-ast/src/token.rs Outdated Show resolved Hide resolved
}

#[derive(Clone, Debug, Serialize)]
pub struct ImplicitReturn {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a doc comment outining what this Span is pointing to when we have an implicit return type.

@JoshuaBatty JoshuaBatty requested a review from a team July 2, 2023 23:22
@eureka-cpu
Copy link
Contributor Author

Putting this on hold while I work on FuelLabs/fuelup#457 for the next beta release.

@sdankel
Copy link
Member

sdankel commented Aug 25, 2023

We should rethink this solution in light of #4981

@eureka-cpu
Copy link
Contributor Author

eureka-cpu commented Aug 25, 2023

We should rethink this solution in light of #4981

In this instance, we don't want the span to be none since it would mean that we couldn't point to where the error actually happens

Side note: this problem actually occurred due to essentially a similar implementation of #4981 where the return type could be None and we were replacing the span of what would be the return type span with the span of the entire function signature. In my main comment you can see what Rust does and how this PR aims to achieve that.

@eureka-cpu eureka-cpu changed the title Fix spans for functions that return implicitly Add Delimiter tokens to the parser Sep 22, 2023
@eureka-cpu
Copy link
Contributor Author

Closing due to how big of a PR this became, and I no longer have time to try and finish it. There is some good stuff in here, for anyone who may decide to tackle the issue of adding delimiters to the parser and how that could affect the resulting AST, error handling and so on.

@eureka-cpu eureka-cpu closed this Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
big this task is hard and will take a while bug Something isn't working compiler: parser Everything to do with the parser
Projects
None yet
4 participants