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

Getters/Setters for fields #1440

Merged
merged 4 commits into from
Apr 30, 2019
Merged

Getters/Setters for fields #1440

merged 4 commits into from
Apr 30, 2019

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Apr 11, 2019

Fixes #222.

@c410-f3r c410-f3r marked this pull request as ready for review April 23, 2019 12:39
@RReverser
Copy link
Member

Is it possible to eliminate the extra method on the JS side and keep only the getter?

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Apr 26, 2019

@RReverser Sure. I was a bit apprehensive about this aspect but I finally came up with the following rules.

Input Output
js_name attr getter/setter with name getter/setter w/o name get/set method name ordinary method name
- - - n/a fn name
- - X fn name n/a
- X - getter/setter name n/a
- X X forbidden¹ n/a
X - - n/a js_name
X - X js_name ignored²
X X - getter/setter name ignored²
X X X forbidden¹ ignored²

¹ Duplicated getters or duplicated setters
² Ordinary method is ignored when the js_name attribute is used together with the getter/setter attribute

@RReverser
Copy link
Member

Ah, so this extra method is generated only due to js_name in the example above and would be omitted otherwise? That makes sense.

@c410-f3r c410-f3r changed the title Getter/Setter for fields Getters/Setters for fields Apr 29, 2019
@c410-f3r c410-f3r closed this Apr 29, 2019
@c410-f3r c410-f3r reopened this Apr 29, 2019
Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in review, I didn't realize this was ready! Feel free to ping whenever it's ready and we can take a look

method(class, &docs, getter_name, "set ", &js, &ts);
}
}
_ => method(class, &docs, fn_name, static_prfx, &js, &ts),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this return an error for unsupported OperationKind types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should an error be thrown? This line code roughly translates to: Don't do fancy things for anything that isn't a getter or a setter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah I think we should generate an error here because something like #[wasm_bindgen(indexing_getter)] (or something like that) isn't fully supported here

crates/shared/src/lib.rs Outdated Show resolved Hide resolved
tests/wasm/getters_and_setters.rs Outdated Show resolved Hide resolved
tests/wasm/getters_and_setters.rs Show resolved Hide resolved
crates/cli-support/src/js/mod.rs Outdated Show resolved Hide resolved
crates/cli-support/src/js/mod.rs Outdated Show resolved Hide resolved
crates/cli-support/src/js/mod.rs Outdated Show resolved Hide resolved
@alexcrichton alexcrichton merged commit 578d59e into rustwasm:master Apr 30, 2019
@alexcrichton
Copy link
Contributor

Thanks again for this @c410-f3r!

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.

Support defining getters in Rust
3 participants