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

[Merged by Bors] - Restructure lints in multiple crates #2447

Closed
wants to merge 11 commits into from

Conversation

raskad
Copy link
Member

@raskad raskad commented Nov 18, 2022

This Pull Request restructures the lint deny/warn/allow lists in almost all crates. boa_engine will be done in a follow up PR as the changes there are pretty extensive.

@raskad raskad added the Internal Category for changelog label Nov 18, 2022
@raskad raskad added this to the v0.17.0 milestone Nov 18, 2022
@github-actions
Copy link

Test262 conformance changes

Test result main count PR count difference
Total 93,811 93,811 0
Passed 69,620 69,620 0
Ignored 18,452 18,452 0
Failed 5,739 5,739 0
Panics 0 0 0
Conformance 74.21% 74.21% 0.00%

@codecov
Copy link

codecov bot commented Nov 18, 2022

Codecov Report

Merging #2447 (f2fe32f) into main (6c3a9a5) will increase coverage by 0.19%.
The diff coverage is 58.82%.

@@            Coverage Diff             @@
##             main    #2447      +/-   ##
==========================================
+ Coverage   52.44%   52.63%   +0.19%     
==========================================
  Files         331      330       -1     
  Lines       35019    34853     -166     
==========================================
- Hits        18365    18346      -19     
+ Misses      16654    16507     -147     
Impacted Files Coverage Δ
boa_cli/src/main.rs 0.00% <0.00%> (-1.52%) ⬇️
boa_gc/src/lib.rs 100.00% <ø> (ø)
boa_gc/src/pointers/ephemeron.rs 92.10% <ø> (ø)
boa_gc/src/pointers/gc.rs 71.13% <ø> (ø)
boa_gc/src/trace.rs 80.39% <ø> (ø)
boa_interner/src/lib.rs 78.99% <0.00%> (-5.05%) ⬇️
boa_interner/src/raw.rs 91.13% <ø> (ø)
boa_interner/src/sym.rs 100.00% <ø> (ø)
boa_parser/src/lexer/comment.rs 89.74% <ø> (ø)
boa_parser/src/lexer/error.rs 58.33% <ø> (ø)
... and 61 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

boa_gc/src/internals/mod.rs Outdated Show resolved Hide resolved
)]
#![allow(clippy::let_unit_value, clippy::module_name_repetitions)]

extern crate self as boa_gc;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think yes, because of the usages of the Trace and Finalize derive macros in boa_gc itself. The macros expect that ::boa::gc is available.

boa_interner/src/lib.rs Show resolved Hide resolved
boa_interner/src/lib.rs Show resolved Hide resolved
@@ -100,12 +125,12 @@ impl Error {
}

/// Creates a "general" parsing error.
pub(crate) fn general(message: &'static str, position: Position) -> Self {
pub(crate) const fn general(message: &'static str, position: Position) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

I would also add a "must use" and inline hint in these functions

Copy link
Member Author

Choose a reason for hiding this comment

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

The lint only marks pub functions. I think for internal function must_use would be a much.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, and we can use the "unused" lint to check if we aren't using results.

boa_parser/src/error.rs Outdated Show resolved Hide resolved
@@ -14,9 +14,10 @@ pub(super) struct Cursor<R> {
impl<R> Cursor<R> {
/// Gets the current position of the cursor in the source code.
#[inline]
pub(super) fn pos(&self) -> Position {
pub(super) const fn pos(&self) -> Position {
Copy link
Member

Choose a reason for hiding this comment

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

This should probably also be must_use right? Does the lint not mark it?

Copy link
Member Author

Choose a reason for hiding this comment

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

See above regarding must_use.

boa_parser/src/lexer/identifier.rs Show resolved Hide resolved
@@ -22,8 +21,8 @@ pub(super) struct SpreadLiteral;

impl SpreadLiteral {
/// Creates a new string literal lexer.
pub(super) fn new() -> Self {
Self {}
pub(super) const fn new() -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

With the way the lexer is built around the Tokenizer trait, I think it is best to stick to the convention and leave this function as is.

boa_parser/src/parser/cursor/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

Thanks for the enhancement! I added some comments for potential improvements :)

clippy::nursery,
)]

use std::fmt::{self, Debug};
Copy link
Member

Choose a reason for hiding this comment

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

This could be included in the std import below

Copy link
Member Author

Choose a reason for hiding this comment

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

The other std import is only enabled when the profiler feature is enabled.

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

Just a small nitpick, and it can be merged :)

mod gc_box;
pub(crate) use gc_box::GcBox;

pub(crate) use {self::ephemeron_box::EphemeronBox, self::gc_box::GcBox};
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better as self::{...}, scoping stuff inside of it, instead of repeating self

@@ -100,12 +125,12 @@ impl Error {
}

/// Creates a "general" parsing error.
pub(crate) fn general(message: &'static str, position: Position) -> Self {
pub(crate) const fn general(message: &'static str, position: Position) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, and we can use the "unused" lint to check if we aren't using results.

@raskad
Copy link
Member Author

raskad commented Nov 19, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 19, 2022
This Pull Request restructures the lint deny/warn/allow lists in almost all crates. `boa_engine` will be done in a follow up PR as the changes there are pretty extensive.
@bors
Copy link

bors bot commented Nov 19, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Restructure lints in multiple crates [Merged by Bors] - Restructure lints in multiple crates Nov 19, 2022
@bors bors bot closed this Nov 19, 2022
@bors bors bot deleted the restructure-lints branch November 19, 2022 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Category for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants