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: function privacy leak validator #338

Merged
merged 1 commit into from Jul 20, 2021

Conversation

mkhan45
Copy link
Contributor

@mkhan45 mkhan45 commented Jul 15, 2021

Part of #309

Adds a validation for leaking private types from function signatures


param_types
.chain(std::iter::once(ret_ty))
.filter(type_is_not_allowed)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add .take(1) to show only a single diagnostic per function

@baszalmstra baszalmstra requested review from Wodann and baszalmstra and removed request for Wodann July 16, 2021 14:35
@baszalmstra
Copy link
Collaborator

Thanks for this!

Would it be possible to highlight the region of the type thas has a more private type? So in the case of:

pub fn foo(ty: PrivateType) { 
               ~~~~~~~~~~~ <-- this part is highlighted: cant leak `PrivateType` 
 /// ...
}

@mkhan45
Copy link
Contributor Author

mkhan45 commented Jul 17, 2021

Alright, I've changed the diagnostic to highlight the type, but it's a bit harder to make it actually display the type name in the diagnostic message. I don't think it's a big deal since the name is highlighted anyway though.

error: can't leak private type
 --> mod.mun:3:16
  |
3 | pub fn foo(ty: PrivateType) -> PrivateType {
  |                ^^^^^^^^^^^ can't leak private type
  |error: can't leak private type
 --> mod.mun:3:32
  |
3 | pub fn foo(ty: PrivateType) -> PrivateType {
  |                                ^^^^^^^^^^^ can't leak private type

@baszalmstra
Copy link
Collaborator

baszalmstra commented Jul 17, 2021

You can implement something in mun_diagnostic to also be able to give proper feedback on the name. In that crate you conversion happens from mun_hir::Diagnostics to mun_diagnostic::Diagnostic. A lot more context is known at that point (like the entire HIR database) which should make it fairly easy to get the name of the type. See https://github.com/mun-lang/mun/blob/master/crates/mun_diagnostics/src/hir/unresolved_type.rs as an example.

The goal of mun_diagnostic is also to provide fixes in the future, this would be a prime candidate.

Adding the name is not required perse but I think it will provide users with a much better experience down the road.

@baszalmstra
Copy link
Collaborator

Also works properly with the language server:

image

Nice!

@baszalmstra baszalmstra requested a review from Wodann July 17, 2021 12:49
@codecov
Copy link

codecov bot commented Jul 17, 2021

Codecov Report

Merging #338 (f0a27e3) into master (1eb2a56) will increase coverage by 0.06%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #338      +/-   ##
==========================================
+ Coverage   80.24%   80.30%   +0.06%     
==========================================
  Files         258      259       +1     
  Lines       15572    15637      +65     
==========================================
+ Hits        12496    12558      +62     
- Misses       3076     3079       +3     
Impacted Files Coverage Δ
crates/mun_codegen/src/test.rs 100.00% <ø> (ø)
crates/mun_hir/src/ty/lower.rs 79.38% <0.00%> (-2.51%) ⬇️
crates/mun_runtime/tests/hot_reloading.rs 100.00% <ø> (ø)
crates/mun_runtime/tests/marshalling.rs 98.20% <ø> (ø)
crates/mun_runtime/tests/memory.rs 100.00% <ø> (ø)
crates/mun_runtime_capi/src/tests.rs 99.49% <ø> (ø)
crates/mun_hir/src/ty.rs 73.75% <75.00%> (+0.06%) ⬆️
crates/mun_hir/src/diagnostics.rs 64.87% <83.33%> (+0.46%) ⬆️
crates/mun_diagnostics/src/hir/exported_private.rs 92.30% <92.30%> (ø)
crates/mun_hir/src/expr/validator.rs 93.84% <92.30%> (-1.03%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1eb2a56...f0a27e3. Read the comment docs.

@mkhan45
Copy link
Contributor Author

mkhan45 commented Jul 17, 2021

I've added a mun_diagnostic diagnostic so the type names show: https://github.com/mun-lang/mun/pull/338/files#diff-f6d9d08feaa1fdc33c5467abad598c17045b35ab9dc93d19b1bf0acf8e61fa6c

Copy link
Collaborator

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

Nice! 👍

Copy link
Collaborator

@Wodann Wodann left a comment

Choose a reason for hiding this comment

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

Looks good overall. I had one question


impl Diagnostic for ExportedPrivate {
fn message(&self) -> String {
"can't leak private type".to_string()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have duplication of this error message. Is there a way to reuse it to avoid code+paste errors?

Other occurance:
https://github.com/mun-lang/mun/pull/338/files#diff-7a57632f7acc9a0b1abad5ef6f9447f95fabecf03312d0ab7d0597807ae8e25dR24

@mkhan45
Copy link
Contributor Author

mkhan45 commented Jul 20, 2021

I've updated it so that there's no duplication

Added a validation for privacy leaks in function signatures, added a few tests, and updated existing tests/examples
@Wodann Wodann merged commit a236a8f into mun-lang:master Jul 20, 2021
@baszalmstra baszalmstra changed the title Function privacy leak validator feat: function privacy leak validator Jul 21, 2021
@Wodann Wodann added this to the Mun v0.4.0 milestone Jul 8, 2022
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.

None yet

3 participants