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 parsed_to_ast fn in sway_core #2210

Merged
merged 2 commits into from
Jul 4, 2022
Merged

Add parsed_to_ast fn in sway_core #2210

merged 2 commits into from
Jul 4, 2022

Conversation

JoshuaBatty
Copy link
Member

This PR refactors compile_to_ast and adds a parsed_to_ast function. This avoids reparsing the whole program again in the language server when doing the type-checking phase as we can call parsed_to_ast with our already obtained CompileResult<ParseProgram>.

@JoshuaBatty JoshuaBatty self-assigned this Jul 4, 2022
@JoshuaBatty JoshuaBatty added language server LSP server compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler: parser Everything to do with the parser labels Jul 4, 2022
@JoshuaBatty JoshuaBatty requested review from a team and adlerjohn July 4, 2022 04:45
otrho
otrho previously approved these changes Jul 4, 2022
Copy link
Contributor

@otrho otrho left a comment

Choose a reason for hiding this comment

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

We have a couple of these little functions now, like parsed_to_ast, compile_to_ast and compile_to_asm. We should look at cleaning up the public interface -- it seems every little step in the pipeline is useful to either tooling or testing, so they all might as well be publicly exposed.

@JoshuaBatty JoshuaBatty requested a review from mitchmindtree July 4, 2022 05:27
@JoshuaBatty JoshuaBatty enabled auto-merge (squash) July 4, 2022 05:29
pub fn compile_to_ast(
input: Arc<str>,
pub fn parsed_to_ast(
parsed: CompileResult<ParseProgram>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be:

Suggested change
parsed: CompileResult<ParseProgram>,
parsed: ParseProgram,

To make this change, we'll also need to change the compile_to_ast body to:

    parse(input, build_config).flat_map(|parsed| {
        parsed_to_ast(parsed, initial_namespace)
    })

The CompileResult's flat_map method should probably be called and_then to better align with the std Option and Result methods, but that's a future PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

But don't we still want to carry any errors and warnings across from the parsing stage?

Copy link
Contributor

Choose a reason for hiding this comment

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

The result returned from sway_core::parse should have already been handled by the user as they likely require access to the ParseProgram within the result (otherwise they would have just called compile_to_ast in the first place).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @mitchmindtree I've changed parsed_to_ast to take a ParseProgram instead of a CompileResult<ParseProgram>.

I've also tried to retain the original behaviour of carrying forward any warnings and errors found during the parsing stage when calling compile_to_ast.

Copy link
Contributor

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

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

Agh I forgot CompileAstResult isn't just an alias for CompileResult<TypedProgram>, so you couldn't have used the CompileResult::flat_map method anyway.

🚢

@otrho
Copy link
Contributor

otrho commented Jul 4, 2022

Yeah, it gets a little complicated in lib.rs as we have 2 or 3? different 'result' types and error types. Again, might be worth revisiting in a future refactoring.

Copy link
Contributor

@otrho otrho left a comment

Choose a reason for hiding this comment

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

You might be able to use check!() in compile_to_ast rather than destructuring the result from parse() but that doesn't let you dedup the errors... but that might happen later anyway? I can't be bothered checking. This is fine. 🙂

@JoshuaBatty JoshuaBatty merged commit 4c17e3d into master Jul 4, 2022
@JoshuaBatty JoshuaBatty deleted the josh/parsed_to_ast branch July 4, 2022 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler: parser Everything to do with the parser language server LSP server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants