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(compiler): access modifiers #4133

Merged
merged 71 commits into from
Sep 15, 2023
Merged

feat(compiler): access modifiers #4133

merged 71 commits into from
Sep 15, 2023

Conversation

yoav-steinberg
Copy link
Contributor

@yoav-steinberg yoav-steinberg commented Sep 10, 2023

See #108
This PR adds public and protected support to class members and while doing so makes all other members private by default. It also verifies interface method implementations must be public. It also includes support for JSII access modifiers. To get this working I did the following refactors:

  • Cleaned up fields in the SymbolEnv and instead added a kind field distinguishing between 3 types of environments: a simple code block (Scope) a function/method/closure environemt (Function) and an environment used to store members of a type (Type). This was primarily needed so I can use the result of a symbol env lookup to figure out in what ancestor class a given member was defined. The added benefit was code clarity and sanity checks which, btw, found a non-related bug in super constructor type checking (see below). One piece of such clarity was the long awaited removal of return_type from SymbolEnv.
  • Cleaned up the ContextVisitor which I needed to add to the TypeChecker. Here I removed the requirement to store the init_env in push_class which didn't make sense, and is probably a leftover of some iteration on our lifting code. I also cleaned up the push_method code to make more sense and now it's a more general push_function stack with a helper for when we're just interested in methods.
  • I added some more stuff to VisitorWithContext trait, for easy "push -> do stuff or even return -> pop" pattern. Specifically I needed the TypeChecker to push function definitions and current statement safely.
  • Use a VisitorContext in TypeChecker to track context so I know in what class I am for access modifier checking. This approach also provides a better way of tracking the current return_type so I could remove it from the env. This also removed all sorts of dirt from the TypeChecker struct (like in_json, etc.).
  • The type_check_statement method was getting way to big, so I moved the contents of its match statements into separate methods.
  • The sanity checks when creating new SymbolEnvs detected that we're creating a weird env in type_check_super_constructor_against_parent_initializer. I removed a large bunch of code there that didn't seem correct to me.
  • The actual access modifier verification takes place inside get_property_from_class_like (<= this is where you want to look). This way I avoid cascading errors and "unhydrated" classes. To make this work for all cases I updated resolve_referece for static type references to also use this method instead of doing its own thing. I think this is cleaner.

Still left and possible candidate for followup PRs:

Checklist

  • Title matches Winglang's style guide
  • Description explains motivation and solution
  • Tests added (always)
  • Docs updated (only required for features)
  • Added pr/e2e-full label if this feature requires end-to-end testing

By submitting this pull request, I confirm that my contribution is made under the terms of the Wing Cloud Contribution License.

@yoav-steinberg yoav-steinberg requested a review from a team as a code owner September 10, 2023 11:11
@monadabot
Copy link
Contributor

monadabot commented Sep 10, 2023

Console preview environment is available at https://wing-console-pr-4133.fly.dev 🚀

Updated (UTC): 2023-09-15 18:12

@yoav-steinberg yoav-steinberg linked an issue Sep 14, 2023 that may be closed by this pull request
@yoav-steinberg yoav-steinberg added the 🚧 pr/do-not-merge PRs with this label will not be automatically merged by mergify. label Sep 14, 2023
@yoav-steinberg yoav-steinberg removed 🚧 pr/do-not-merge PRs with this label will not be automatically merged by mergify. ⚠️ pr/review-mutation PR has been mutated and will not auto-merge. Clear this label if the changes look good! labels Sep 14, 2023
libs/wingsdk/src/.gen/versions.json Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Sep 15, 2023

Thanks for contributing, @yoav-steinberg! This PR will now be added to the merge queue, or immediately merged if yoav/access_modifiers is up-to-date with main and the queue is empty.

mergify bot added a commit that referenced this pull request Sep 15, 2023
@mergify mergify bot merged commit 3f09a3e into main Sep 15, 2023
3 checks passed
@mergify mergify bot deleted the yoav/access_modifiers branch September 15, 2023 18:38
@monadabot
Copy link
Contributor

Congrats! 🚀 This was released in Wing 0.30.9.

skorfmann added a commit to winglang/examples that referenced this pull request Sep 19, 2023
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.

Access Modifiers
4 participants