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

feat: Added type hierarchy support for arrays and slices #1957

Closed
wants to merge 6 commits into from

Conversation

sirasistant
Copy link
Contributor

@sirasistant sirasistant commented Jul 18, 2023

Description

Problem*

Arrays are a subtype of slice and SSA & brillig gen wasn't handling that properly.

Summary*

This adds support for type hierarchy handling in defunctionalization and in brillig gen. Also fixes various bugs found along the way. The changes to support type hierarchy itself are contained so it should be easy to remove when we remove type hierarchy from the SSA. In the meanwhile, this PR should allow us to continue advancing in other tasks.

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@sirasistant sirasistant changed the title feat: added type hierarchy support for arrays and slices feat: Added type hierarchy support for arrays and slices Jul 18, 2023
@sirasistant sirasistant changed the title feat: Added type hierarchy support for arrays and slices feat: Added type hierarchy support for arrays and slices Jul 18, 2023
@sirasistant
Copy link
Contributor Author

I'm working on an approach that I think can be better, only casting as needed and only in function calls

@sirasistant sirasistant marked this pull request as draft July 18, 2023 13:44
@jfecher
Copy link
Contributor

jfecher commented Jul 18, 2023

If we decide against type hierarchy in SSA, we can just revert this PR.

This wouldn't be needed then if arrays were not subtypes of slices? Perhaps we should just remove that and make them distinct types. Separating their syntax as well.

@sirasistant sirasistant force-pushed the arv/type_slice_type_hierarchy branch from 522feee to c539c2d Compare July 18, 2023 16:55
@sirasistant sirasistant marked this pull request as ready for review July 19, 2023 07:50
@sirasistant
Copy link
Contributor Author

sirasistant commented Jul 19, 2023

This PR can be used as a stopgap until #1963 is finalized

@jfecher
Copy link
Contributor

jfecher commented Jul 19, 2023

This PR can be used as a stopgap until #1963 is finalized

I don't think we should merge PRs if they're only going to be used temporarily while we explore something else. Unless that other thing is quite a long time away and the stopgap would be both non-breaking and easy to remove. I'll work on #1963 today if getting this fixed quickly is important.

@sirasistant
Copy link
Contributor Author

The stopgap should be non-breaking and removing it should be as simple as removing the is method in SSA::Type, the usage of those casts in defunctionalization::CallSignature::can_call and the three cast methods in brillig_block. This PR includes other changes that are fixes to issues found testing this and other improvements. I will separate this in two PRs!

@sirasistant sirasistant force-pushed the arv/type_slice_type_hierarchy branch from 1f37820 to e97da64 Compare July 19, 2023 15:37
@sirasistant sirasistant changed the base branch from master to arv/various_fixes July 19, 2023 15:38
@sirasistant
Copy link
Contributor Author

separated into this one and #1973

Base automatically changed from arv/various_fixes to master July 19, 2023 20:37
@kevaundray
Copy link
Contributor

@jfecher and @sirasistant where did we leave off in regards to this PR?

@jfecher
Copy link
Contributor

jfecher commented Aug 7, 2023

It was my impression this PR is no longer needed after #1963 was merged. Pinging @sirasistant for confirmation

@sirasistant
Copy link
Contributor Author

Not needed anymore after 1963, closing!

@sirasistant sirasistant closed this Aug 8, 2023
@kevaundray kevaundray deleted the arv/type_slice_type_hierarchy branch August 8, 2023 07:29
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.

3 participants