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

add clippy linting CI target #458

Merged
merged 2 commits into from
Dec 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -137,3 +137,21 @@ jobs:
with:
components: rustfmt
- run: cargo fmt --all -- --check --unstable-features --error-on-unformatted

clippy:
name: lint
runs-on: ubuntu-latest
steps:
- name: Install Cap'n Proto
run: |
export DEBIAN_FRONTEND=noninteractive
sudo apt-get install -y capnproto libcapnp-dev

- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@master
with:
toolchain: nightly-2023-11-30
components: clippy
- uses: actions-rs-plus/clippy-check@v2
with:
args: --all --all-targets
4 changes: 3 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,6 @@ members = [
default-members = [
"capnp",
"capnpc",
]
]

[workspace.lints]
2 changes: 2 additions & 0 deletions async-byte-channel/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,5 @@ version = "0.3.0"
default-features = false
features = ["std", "executor"]

[lints]
workspace = true
3 changes: 3 additions & 0 deletions capnp-futures/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,6 @@ features = ["executor"]
[dev-dependencies]
capnp = { version = "0.18.0", path = "../capnp", features = ["quickcheck"] }
quickcheck = "1"

[lints]
workspace = true
6 changes: 6 additions & 0 deletions capnp-rpc/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,9 @@ features = ["std"]
[dependencies]
capnp-futures = { version = "0.18.0", path = "../capnp-futures" }
capnp = {version = "0.18.0", path = "../capnp"}

#[lints]
#workspace = true
Copy link
Member

Choose a reason for hiding this comment

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

Why commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as above. this should be uncommented as soon as the type complexity lint is addressed.

Would you prefer i remove the commented sections?


[lints.clippy]
type_complexity = "allow" # this should be removed in future
122 changes: 62 additions & 60 deletions capnp-rpc/src/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -682,7 +682,7 @@

fn send_unimplemented(
connection_state: &Rc<Self>,
message: &Box<dyn crate::IncomingMessage>,
message: &dyn crate::IncomingMessage,

Check warning on line 685 in capnp-rpc/src/rpc.rs

View check run for this annotation

Codecov / codecov/patch

capnp-rpc/src/rpc.rs#L685

Added line #L685 was not covered by tests
) -> capnp::Result<()> {
let mut out_message = connection_state.new_outgoing_message(50)?; // XXX size hint
{
Expand Down Expand Up @@ -1031,61 +1031,65 @@
Some(ref mut question) => {
question.is_awaiting_return = false;
match question.self_ref {
Some(ref question_ref) => {
match ret.which()? {
return_::Results(results) => {
let cap_table = Self::receive_caps(
&connection_state,
results?.get_cap_table()?,
)?;

let question_ref =
question_ref.upgrade().expect("dangling question ref?");
let response = Response::new(
connection_state.clone(),
question_ref.clone(),
message,
cap_table,
);
question_ref.borrow_mut().fulfill(Promise::ok(response));
}
return_::Exception(e) => {
let tmp =
question_ref.upgrade().expect("dangling question ref?");
tmp.borrow_mut().reject(remote_exception_to_error(e?));
}
return_::Canceled(_) => {
Self::send_unimplemented(&connection_state, &message)?;
}
return_::ResultsSentElsewhere(_) => {
Self::send_unimplemented(&connection_state, &message)?;
}
return_::TakeFromOtherQuestion(id) => {
if let Some(answer) =
connection_state.answers.borrow_mut().slots.get_mut(&id)
{
if let Some(res) = answer.redirected_results.take() {
let tmp = question_ref
.upgrade()
.expect("dangling question ref?");
tmp.borrow_mut().fulfill(res);
} else {
return Err(Error::failed("return.takeFromOtherQuestion referenced a call that \
did not use sendResultsTo.yourself.".to_string()));
}
Some(ref question_ref) => match ret.which()? {
return_::Results(results) => {
let cap_table = Self::receive_caps(
&connection_state,
results?.get_cap_table()?,
)?;

Check warning on line 1039 in capnp-rpc/src/rpc.rs

View check run for this annotation

Codecov / codecov/patch

capnp-rpc/src/rpc.rs#L1039

Added line #L1039 was not covered by tests

let question_ref =
question_ref.upgrade().expect("dangling question ref?");
let response = Response::new(
connection_state.clone(),
question_ref.clone(),
message,
cap_table,
);
question_ref.borrow_mut().fulfill(Promise::ok(response));
}
return_::Exception(e) => {
let tmp =
question_ref.upgrade().expect("dangling question ref?");
tmp.borrow_mut().reject(remote_exception_to_error(e?));
}
return_::Canceled(_) => {
Self::send_unimplemented(&connection_state, message.as_ref())?;

Check warning on line 1057 in capnp-rpc/src/rpc.rs

View check run for this annotation

Codecov / codecov/patch

capnp-rpc/src/rpc.rs#L1057

Added line #L1057 was not covered by tests
danieleades marked this conversation as resolved.
Show resolved Hide resolved
}
return_::ResultsSentElsewhere(_) => {
Self::send_unimplemented(&connection_state, message.as_ref())?;

Check warning on line 1060 in capnp-rpc/src/rpc.rs

View check run for this annotation

Codecov / codecov/patch

capnp-rpc/src/rpc.rs#L1060

Added line #L1060 was not covered by tests
}
return_::TakeFromOtherQuestion(id) => {
if let Some(answer) =
connection_state.answers.borrow_mut().slots.get_mut(&id)

Check warning on line 1064 in capnp-rpc/src/rpc.rs

View check run for this annotation

Codecov / codecov/patch

capnp-rpc/src/rpc.rs#L1062-L1064

Added lines #L1062 - L1064 were not covered by tests
{
if let Some(res) = answer.redirected_results.take() {
let tmp = question_ref
.upgrade()
.expect("dangling question ref?");
tmp.borrow_mut().fulfill(res);

Check warning on line 1070 in capnp-rpc/src/rpc.rs

View check run for this annotation

Codecov / codecov/patch

capnp-rpc/src/rpc.rs#L1066-L1070

Added lines #L1066 - L1070 were not covered by tests
} else {
return Err(Error::failed("return.takeFromOtherQuestion had invalid answer ID.".to_string()));
return Err(Error::failed("return.takeFromOtherQuestion referenced a call that \
did not use sendResultsTo.yourself.".to_string()));

Check warning on line 1073 in capnp-rpc/src/rpc.rs

View check run for this annotation

Codecov / codecov/patch

capnp-rpc/src/rpc.rs#L1072-L1073

Added lines #L1072 - L1073 were not covered by tests
}
}
return_::AcceptFromThirdParty(_) => {
drop(questions);
Self::send_unimplemented(&connection_state, &message)?;
} else {
return Err(Error::failed(
"return.takeFromOtherQuestion had invalid answer ID."
.to_string(),
));

Check warning on line 1079 in capnp-rpc/src/rpc.rs

View check run for this annotation

Codecov / codecov/patch

capnp-rpc/src/rpc.rs#L1076-L1079

Added lines #L1076 - L1079 were not covered by tests
}
}
}
return_::AcceptFromThirdParty(_) => {
drop(questions);
Self::send_unimplemented(&connection_state, message.as_ref())?;

Check warning on line 1084 in capnp-rpc/src/rpc.rs

View check run for this annotation

Codecov / codecov/patch

capnp-rpc/src/rpc.rs#L1083-L1084

Added lines #L1083 - L1084 were not covered by tests
}
},
None => {
if let return_::TakeFromOtherQuestion(_) = ret.which()? {
return Self::send_unimplemented(&connection_state, &message);
return Self::send_unimplemented(
&connection_state,
message.as_ref(),
);

Check warning on line 1092 in capnp-rpc/src/rpc.rs

View check run for this annotation

Codecov / codecov/patch

capnp-rpc/src/rpc.rs#L1089-L1092

Added lines #L1089 - L1092 were not covered by tests
}
// Looks like this question was canceled earlier, so `Finish`
// was already sent, with `releaseResultCaps` set true so that
Expand Down Expand Up @@ -1155,7 +1159,7 @@
| message::ObsoleteDelete(_),
)
| Err(::capnp::NotInSchema(_)) => {
Self::send_unimplemented(&connection_state, &message)?;
Self::send_unimplemented(&connection_state, message.as_ref())?;

Check warning on line 1162 in capnp-rpc/src/rpc.rs

View check run for this annotation

Codecov / codecov/patch

capnp-rpc/src/rpc.rs#L1162

Added line #L1162 was not covered by tests
}
}
Ok(())
Expand Down Expand Up @@ -1283,8 +1287,7 @@
}
}

fn get_innermost_client(&self, client_ref: &Box<dyn ClientHook>) -> Box<dyn ClientHook> {
let mut client = client_ref.clone();
fn get_innermost_client(&self, mut client: Box<dyn ClientHook>) -> Box<dyn ClientHook> {
while let Some(inner) = client.get_resolved() {
client = inner;
}
Expand Down Expand Up @@ -1314,7 +1317,7 @@

match resolution_result {
Ok(resolution) => {
let resolution = connection_state.get_innermost_client(&resolution);
let resolution = connection_state.get_innermost_client(resolution.clone());

let brand = resolution.get_brand();

Expand Down Expand Up @@ -1353,7 +1356,7 @@
resolve.set_promise_id(export_id);
let _export = Self::write_descriptor(
&connection_state,
&resolution,
resolution,
resolve.init_cap(),
)?;
}
Expand All @@ -1378,11 +1381,10 @@

fn write_descriptor(
state: &Rc<Self>,
cap: &Box<dyn ClientHook>,
mut inner: Box<dyn ClientHook>,
mut descriptor: cap_descriptor::Builder,
) -> ::capnp::Result<Option<ExportId>> {
// Find the innermost wrapped capability.
let mut inner = cap.clone();
while let Some(resolved) = inner.get_resolved() {
inner = resolved;
}
Expand Down Expand Up @@ -1439,10 +1441,10 @@
let mut exports = Vec::new();
for (idx, value) in cap_table.iter().enumerate() {
match value {
Some(ref cap) => {
Some(cap) => {
if let Some(export_id) = Self::write_descriptor(
state,
cap,
cap.clone(),
cap_table_builder.reborrow().get(idx as u32),
)
.unwrap()
Expand Down Expand Up @@ -2972,7 +2974,7 @@

ConnectionState::write_descriptor(
&self.connection_state.clone(),
&promise_client.borrow().cap.clone(),
promise_client.borrow().cap.clone(),
descriptor,
)
.unwrap()
Expand Down
6 changes: 2 additions & 4 deletions capnp-rpc/test/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -736,12 +736,10 @@ fn embargo_success() {
call5.promise,
])
.map(|responses| {
let mut counter = 0;
for r in responses?.into_iter() {
if counter != r.get()?.get_n() {
for (counter, r) in responses?.into_iter().enumerate() {
if counter != r.get()?.get_n() as usize {
return Err(Error::failed("calls arrived out of order".to_string()));
}
counter += 1;
}
Ok(())
})
Expand Down
7 changes: 7 additions & 0 deletions capnp/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,10 @@ std = ["embedded-io?/std"]
# message readers to be `Sync`. Note that AtomicUsize is not supported by all
# rustc targets.
sync_reader = []

#[lints]
#workspace = true
danieleades marked this conversation as resolved.
Show resolved Hide resolved

[lints.clippy]
type_complexity = "allow" # this should be removed in future
missing_safety_doc = "allow" # this should be removed in future
1 change: 1 addition & 0 deletions capnp/src/private/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2146,6 +2146,7 @@ mod wire_helpers {
}
}

#[allow(clippy::too_many_arguments)]
pub unsafe fn copy_pointer(
dst_arena: &mut dyn BuilderArena,
dst_segment_id: u32,
Expand Down
3 changes: 3 additions & 0 deletions capnpc/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,6 @@ path = "src/capnpc-rust-bootstrap.rs"
[dependencies.capnp]
version = "0.18.0"
path = "../capnp"

[lints]
workspace = true
1 change: 1 addition & 0 deletions capnpc/test/dynamic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ fn test_generics() {
let root: dynamic_value::Builder<'_> = root.into();
let mut root: dynamic_struct::Builder<'_> = root.downcast();

#[allow(clippy::disallowed_names)]
let foo = root.reborrow().get_named("foo").unwrap();
test_util::dynamic_init_test_message(foo.downcast());

Expand Down
3 changes: 3 additions & 0 deletions example/addressbook/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,6 @@ path = "../../capnpc"

[dependencies.capnp]
path = "../../capnp"

[lints]
workspace = true
3 changes: 3 additions & 0 deletions example/addressbook_send/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,6 @@ capnpc = { path = "../../capnpc" }

[dependencies]
capnp = { path = "../../capnp" }

[lints]
workspace = true
5 changes: 4 additions & 1 deletion example/fill_random_values/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,7 @@ path = "fill_addressbook.rs"

[[bin]]
name = "fill_shapes"
path = "fill_shapes.rs"
path = "fill_shapes.rs"

[lints]
workspace = true
3 changes: 3 additions & 0 deletions example/wasm-hello-world/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,6 @@ path = "../../capnp"

[build-dependencies.capnpc]
path = "../../capnpc"

[lints]
workspace = true
Loading