Skip to content

[AccessBase] FindAccessBaseVisitor visits nested accesses. #68389

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

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

nate-chandler
Copy link
Contributor

@nate-chandler nate-chandler commented Sep 8, 2023

Implemented visitNestedAccess in FindAccessBaseVisitor to visit the value found by the superclass. Doing so is required by AccessPhiVisitor.

rdar://115033825

@nate-chandler nate-chandler force-pushed the rdar115033825 branch 2 times, most recently from d37a67e to f0ca3af Compare September 8, 2023 00:45
Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

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

Thanks!

The initial thinking was that we could never see a begin_access after following a phi. That's not actually enforceable and we should end up being conservative anyway if we care about the access scope. So, this fix is correct.

But... I stared at this source for a long time to try to prove that it's complete. I mean, a very long time.

It all rests on the fact that all of the other implementations of FindAccessVisitorImpl happen to use NestedAccessType::IgnoreAccessBegin.

I think we need to leave some breadcrumb about what we've learned. For example

Why doesn't FindAccessVisitorImpl::visitNestedAccess always do this itself in the StopAtAccessBegin case`?

Or if that doesn't work, at least comment in the other visitors why they don't need to overried visitNestedAccess.

Or some other idea?

@nate-chandler
Copy link
Contributor Author

Yeah, it's incomplete. Like you say, the other subclasses would need to be updated or the superclass would. I've come around to updating the superclass to implement visitNestedAccess as in this patch except instead of calling SuperTy::visitNestedAccess it either directly looks at the two cases (what's nestedAccessTy) or calls a renaming of the current impl of visitNestedAccess.

Implemented visitNestedAccess in FindAccessBaseVisitor to visit the
value found by the superclass.

rdar://115033825
@nate-chandler
Copy link
Contributor Author

@swift-ci please test

@atrick
Copy link
Contributor

atrick commented Sep 8, 2023

Yeah, it's incomplete. Like you say, the other subclasses would need to be updated or the superclass would. I've come around to updating the superclass to implement visitNestedAccess as in this patch except instead of calling SuperTy::visitNestedAccess it either directly looks at the two cases (what's nestedAccessTy) or calls a renaming of the current impl of visitNestedAccess.

Nice! I think that's better.

@nate-chandler nate-chandler merged commit 2df6e80 into swiftlang:main Sep 8, 2023
@nate-chandler nate-chandler deleted the rdar115033825 branch September 8, 2023 14:04
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