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

Introduce the collection context in the type check context. #6460

Merged

Conversation

tritao
Copy link
Contributor

@tritao tritao commented Aug 23, 2024

Description

This PR does a couple things:

  • Creates all of the necessary lexical scopes in the collection context namespace
  • Introduces the collection context in the type check context
  • Updates type checking code to enter the corresponding lexical scope in the collection context
  • Introduces a mapping from a span to a lexical scope in the namespace
  • Implements collection steps for so far unimplemented AST nodes

This prepares the compiler for a future PR where the additional namespace will be able to be removed from the type checking, and with it the cloning we do as we type check.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@tritao tritao added compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler: collection Everything to do with graph collection, type collection, and the collection context. labels Aug 23, 2024
@tritao tritao self-assigned this Aug 23, 2024
@tritao tritao force-pushed the collection-context-in-type-check-context branch 4 times, most recently from 61d53a9 to 20b93ce Compare August 28, 2024 13:29
@tritao tritao marked this pull request as ready for review August 28, 2024 13:55
@tritao tritao requested a review from a team as a code owner August 28, 2024 13:55
IGI-111
IGI-111 previously approved these changes Sep 2, 2024
@IGI-111 IGI-111 requested a review from a team September 2, 2024 13:37
@tritao tritao force-pushed the collection-context-in-type-check-context branch 2 times, most recently from e76e5ac to 6a8f29c Compare September 16, 2024 19:04
@tritao tritao force-pushed the collection-context-in-type-check-context branch from 6a8f29c to 5101f16 Compare September 16, 2024 19:04
Copy link

codspeed-hq bot commented Sep 16, 2024

CodSpeed Performance Report

Merging #6460 will not alter performance

Comparing tritao:collection-context-in-type-check-context (6749479) with master (613f129)

Summary

✅ 22 untouched benchmarks

@tritao
Copy link
Contributor Author

tritao commented Sep 17, 2024

Updated the PR, rebased on top of the latest master and also some minor improvents to error handling in some cases.

Benchmarks breakdown

Benchmark master tritao:collection-context-in-type-check-context Change
document_symbol 4.3 ms 5.3 ms -18.34%

I have seen this same slowdown in #6553, so I don't think its specific to this.

Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

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

nice one, excited to see this progress.

@JoshuaBatty JoshuaBatty requested a review from a team September 17, 2024 11:47
@tritao tritao merged commit d178613 into FuelLabs:master Sep 19, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler: collection Everything to do with graph collection, type collection, and the collection context. compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants