Skip to content

feat: function privacy leak validator#338

Merged
Wodann merged 1 commit intomun-lang:masterfrom
mkhan45:fix_private_leak
Jul 20, 2021
Merged

feat: function privacy leak validator#338
Wodann merged 1 commit intomun-lang:masterfrom
mkhan45:fix_private_leak

Conversation

@mkhan45
Copy link
Copy Markdown
Contributor

@mkhan45 mkhan45 commented Jul 15, 2021

Part of #309

Adds a validation for leaking private types from function signatures

mkhan45
mkhan45 commented Jul 15, 2021
View reviewed changes
crates/mun_hir/src/expr/validator.rs Outdated

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

@mkhan45 mkhan45 Jul 15, 2021

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
Copy Markdown
Collaborator

baszalmstra commented Jul 16, 2021

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 mkhan45 force-pushed the fix_private_leak branch from 3e72c8b to 36c93d8 Compare July 17, 2021 02:15
@mkhan45
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

baszalmstra commented Jul 17, 2021 •
edited
Loading

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
Copy Markdown
Collaborator

baszalmstra commented Jul 17, 2021

Also works properly with the language server:

image

Nice!

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

codecov bot commented Jul 17, 2021 •
edited
Loading

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 mkhan45 mentioned this pull request Jul 17, 2021
Closed
@mkhan45 mkhan45 force-pushed the fix_private_leak branch from 36c93d8 to 0f52b00 Compare July 17, 2021 22:34
@mkhan45
Copy link
Copy Markdown
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

baszalmstra
baszalmstra approved these changes Jul 18, 2021
View reviewed changes
Copy link
Copy Markdown
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! 👍

Wodann
Wodann reviewed Jul 20, 2021
View reviewed changes
Copy link
Copy Markdown
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

crates/mun_hir/src/diagnostics.rs

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

@Wodann Wodann Jul 20, 2021

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 mkhan45 force-pushed the fix_private_leak branch from 0f52b00 to 070807b Compare July 20, 2021 15:48
mkhan45
mkhan45 commented Jul 20, 2021
View reviewed changes
@mkhan45
Copy link
Copy Markdown
Contributor Author

mkhan45 commented Jul 20, 2021

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

@mkhan45
Feat: Added private type leak validation to functions
f0a27e3 
Added a validation for privacy leaks in function signatures, added a few tests, and updated existing tests/examples
@mkhan45 mkhan45 force-pushed the fix_private_leak branch from 070807b to f0a27e3 Compare July 20, 2021 18:09
@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
@mkhan45 mkhan45 mentioned this pull request Aug 14, 2021
Closed
@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

Reviewers

@baszalmstra baszalmstra baszalmstra approved these changes

@Wodann Wodann Wodann approved these changes

Assignees

@mkhan45 mkhan45

Labels

None yet

Projects

None yet

Milestone

Mun v0.4.0

Development

Successfully merging this pull request may close these issues.

3 participants

@mkhan45 @baszalmstra @Wodann