dnrops
/
smithy-rs
mirror of https://github.com/smithy-lang/smithy-rs
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

More work on cleaning up TODOs (#997) 

* wip

* wip

* Refactor lint tool

* Add a lint to validate that TODOs have context

* cleanup lint some more

* fix codegen failure & remove unused arguments
This commit is contained in:
Russell Cohen 2021-12-21 11:22:43 -05:00 committed by GitHub
parent e38c531464
commit 9f1ef3d408
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
28 changed files with 545 additions and 297 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsEndpointDecorator.kt
View File

@ -257,7 +257,7 @@ class EndpointResolverGenerator(codegenContext: CodegenContext, private val endp
private fun signatureVersion(): String {
val signatureVersions = endpoint.expectArrayMember("signatureVersions").map { it.expectStringNode().value }
// TODO: we can use this to change the signing options instead of customizing S3 specifically
// TODO(https://github.com/awslabs/smithy-rs/issues/977): we can use this to change the signing options instead of customizing S3 specifically
if (!(signatureVersions.contains("v4") || signatureVersions.contains("s3v4"))) {
throw CodegenException("endpoint does not support sigv4, unsupported: $signatureVersions")
}

3
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/customize/glacier/AccountIdAutofill.kt
View File

@ -16,9 +16,6 @@ import software.amazon.smithy.rust.codegen.util.inputShape
class AccountIdAutofill() : OperationCustomization() {
override fun mutSelf(): Boolean = true
// this is a bit of a hack, but there currently isn't a good way to pass the information up the chain into the
// fluent builder. I think we what we actually want is to write this information into the symbol metadata, but TODO.
override fun consumesSelf(): Boolean = true
override fun section(section: OperationSection): Writable {
return when (section) {

2
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/traits/PresignableTrait.kt
View File

@ -10,7 +10,7 @@ import software.amazon.smithy.model.shapes.ShapeId
import software.amazon.smithy.model.traits.AnnotationTrait
/** Synthetic trait that indicates an operation is presignable. */
// TODO: This can be replaced once https://github.com/awslabs/smithy/pull/897 is merged.
// TODO(https://github.com/awslabs/smithy/pull/897) this can be replaced when the trait is added to Smithy.
class PresignableTrait(val syntheticOperationId: ShapeId) : AnnotationTrait(ID, Node.objectNode()) {
companion object {
val ID = ShapeId.from("smithy.api.aws.internal#presignable")

23
aws/sdk/build.gradle.kts
View File

@ -314,29 +314,6 @@ tasks.register<Exec>("cargoClippy") {
dependsOn("assemble")
}
tasks.register<RunExampleTask>("runExample") {
dependsOn("assemble")
outputDir = outputDir
}
// TODO: validate that the example exists. Otherwise this fails with a hidden error.
open class RunExampleTask @javax.inject.Inject constructor() : Exec() {
@Option(option = "example", description = "Example to run")
var example: String? = null
set(value) {
workingDir = workingDir.resolve(value!!)
field = value
}
@org.gradle.api.tasks.InputDirectory
var outputDir: File? = null
set(value) {
workingDir = value!!.resolve("examples")
commandLine = listOf("cargo", "run")
field = value
}
}
tasks["test"].finalizedBy("cargoClippy", "cargoTest", "cargoDocs")
tasks["clean"].doFirst {

2
aws/sdk/integration-tests/kms/tests/integration.rs
View File

@ -18,7 +18,7 @@ use std::time::{Duration, UNIX_EPOCH};
type Client<C> = CoreClient<C, DefaultMiddleware>;
// TODO: having the full HTTP requests right in the code is a bit gross, consider something
// TODO(DVR): having the full HTTP requests right in the code is a bit gross, consider something
// like https://github.com/davidbarsky/sigv4/blob/master/aws-sigv4/src/lib.rs#L283-L315 to store
// the requests/responses externally

2
aws/sdk/integration-tests/qldbsession/tests/integration.rs
View File

@ -17,7 +17,7 @@ use qldbsession::{Config, Region};
use std::time::{Duration, UNIX_EPOCH};
pub type Client<C> = CoreClient<C, DefaultMiddleware>;
// TODO: having the full HTTP requests right in the code is a bit gross, consider something
// TODO(DVR): having the full HTTP requests right in the code is a bit gross, consider something
// like https://github.com/davidbarsky/sigv4/blob/master/aws-sigv4/src/lib.rs#L283-L315 to store
// the requests/responses externally

13
codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/RustTypes.kt
View File

@ -24,7 +24,7 @@ fun autoDeref(input: String) = if (input.startsWith("&")) {
*/
sealed class RustType {
// TODO: when Kotlin supports, sealed interfaces, seal Container
// TODO(kotlin): when Kotlin supports, sealed interfaces, seal Container
/**
* A Rust type that contains [member], another RustType. Used to generically operate over
* shapes that contain other shapes, e.g. [stripOuter] and [contains].
@ -64,7 +64,8 @@ sealed class RustType {
}
data class HashMap(val key: RustType, override val member: RustType) : RustType(), Container {
// TODO: assert that underneath, the member is a String
// validating that `key` is a string occurs in the constructor in SymbolVisitor
override val name: kotlin.String = "HashMap"
override val namespace = "std::collections"
@ -74,11 +75,15 @@ sealed class RustType {
}
data class HashSet(override val member: RustType) : RustType(), Container {
// TODO: assert that underneath, the member is a String
override val name: kotlin.String = Type
override val name = Type
override val namespace = Namespace
companion object {
// This is Vec intentionally. Note the following passage from the Smithy spec:
// Sets MUST be insertion ordered. Not all programming languages that support sets
// support ordered sets, requiring them may be overly burdensome for users, or conflict with language
// idioms. Such languages SHOULD store the values of sets in a list and rely on validation to ensure uniqueness.
// It's possible that we could provide our own wrapper type in the future.
const val Type = "Vec"
const val Namespace = "std::vec"
val RuntimeType = RuntimeType(name = Type, namespace = Namespace, dependency = null)

6
codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/RustWriter.kt
View File

@ -207,7 +207,6 @@ fun <T : CodeWriter> T.rustBlock(
* Generate a RustDoc comment for [shape]
*/
fun <T : CodeWriter> T.documentShape(shape: Shape, model: Model, autoSuppressMissingDocs: Boolean = true, note: String? = null): T {
// TODO: support additional Smithy documentation traits like @example
val docTrait = shape.getMemberTrait(model, DocumentationTrait::class.java).orNull()
when (docTrait?.value?.isNotBlank()) {
@ -248,7 +247,6 @@ fun RustWriter.containerDocs(text: String, vararg args: Any): RustWriter {
fun <T : CodeWriter> T.docs(text: String, vararg args: Any, newlinePrefix: String = "/// "): T {
pushState()
setNewlinePrefix(newlinePrefix)
// TODO: Smithy updates should remove the need for a number of these changes
val cleaned = text.lines()
.joinToString("\n") {
// Rustdoc warns on tabs in documentation
@ -378,7 +376,6 @@ class RustWriter private constructor(
*
*/
fun ifSet(shape: Shape, member: Symbol, outerField: String, block: RustWriter.(field: String) -> Unit) {
// TODO: this API should be refactored so that we don't need to strip `&` to get reference comparisons to work.
when {
member.isOptional() -> {
val derefName = safeName("inner")
@ -419,8 +416,7 @@ class RustWriter private constructor(
prewriter.toString()
} else null
// Hack to support TOML
// TODO: consider creating a TOML writer
// Hack to support TOML: the [commentCharacter] is overridden to support writing TOML.
val header = if (printWarning) {
"$commentCharacter Code generated by software.amazon.smithy.rust.codegen.smithy-rs. DO NOT EDIT."
} else null

1
codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/CodegenDelegator.kt
View File

@ -147,7 +147,6 @@ open class RustCrate(
}
}
// TODO: this should _probably_ be configurable via RustSettings; 2h
/**
* Allowlist of modules that will be exposed publicly in generated crates
*/

1
codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/RuntimeTypes.kt
View File

@ -145,7 +145,6 @@ data class RuntimeType(val name: String?, val dependency: RustDependency?, val n
return "$namespace$postFix"
}
// TODO: refactor to be RuntimeTypeProvider a la Symbol provider that packages the `RuntimeConfig` state.
/**
* The companion object contains commonly used RuntimeTypes
*/

3
codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/SymbolVisitor.kt
View File

@ -48,8 +48,6 @@ import software.amazon.smithy.rust.codegen.util.toPascalCase
import software.amazon.smithy.rust.codegen.util.toSnakeCase
import kotlin.reflect.KClass
// TODO: currently, respecting integer types.
// Should we not? [Go does not]
val SimpleShapes: Map<KClass<out Shape>, RustType> = mapOf(
BooleanShape::class to RustType.Bool,
FloatShape::class to RustType.Float(32),
@ -68,7 +66,6 @@ data class SymbolVisitorConfig(
val handleRustBoxing: Boolean = true
)
// TODO: consider if this is better handled as a wrapper
val DefaultConfig =
SymbolVisitorConfig(
runtimeConfig = RuntimeConfig(),

3
codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/customize/OperationCustomization.kt
View File

@ -53,6 +53,9 @@ sealed class OperationSection(name: String) : Section(name) {
abstract class OperationCustomization : NamedSectionGenerator<OperationSection>() {
open fun retryType(): RuntimeType? = null
// NOTE: mutSelf and consumes self must be set together due to a the fact that the fluent builder does not have any information
// about consumes / mut
/**
* Does `make_operation` consume the self parameter?
*

18
codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/BuilderGenerator.kt
View File

@ -63,7 +63,6 @@ class BuilderGenerator(
fun render(writer: RustWriter) {
val symbol = symbolProvider.toSymbol(shape)
// TODO: figure out exactly what docs we want on a the builder module
writer.docs("See #D", symbol)
val segments = shape.builderSymbol(symbolProvider).namespace.split("::")
writer.withModule(segments.last()) {
@ -108,9 +107,10 @@ class BuilderGenerator(
}
// TODO(EventStream): [DX] Consider updating builders to take EventInputStream as Into<EventInputStream>
private fun renderBuilderMember(writer: RustWriter, member: MemberShape, memberName: String, memberSymbol: Symbol) {
private fun renderBuilderMember(writer: RustWriter, memberName: String, memberSymbol: Symbol) {
// builder members are crate-public to enable using them
// directly in serializers/deserializers
// directly in serializers/deserializers. During XML deserialization, `builder.<field>.take` is used to append to
// lists and maps
writer.write("pub(crate) $memberName: #T,", memberSymbol)
}
@ -118,8 +118,7 @@ class BuilderGenerator(
writer: RustWriter,
coreType: RustType,
member: MemberShape,
memberName: String,
memberSymbol: Symbol
memberName: String
) {
fun builderConverter(coreType: RustType) = when (coreType) {
is RustType.String,
@ -143,8 +142,7 @@ class BuilderGenerator(
writer: RustWriter,
outerType: RustType,
member: MemberShape,
memberName: String,
memberSymbol: Symbol
memberName: String
) {
// Render a `set_foo` method. This is useful as a target for code generation, because the argument type
// is the same as the resulting member type, and is always optional.
@ -170,7 +168,7 @@ class BuilderGenerator(
val memberName = symbolProvider.toMemberName(member)
// All fields in the builder are optional
val memberSymbol = symbolProvider.toSymbol(member).makeOptional()
renderBuilderMember(this, member, memberName, memberSymbol)
renderBuilderMember(this, memberName, memberSymbol)
}
}
@ -186,10 +184,10 @@ class BuilderGenerator(
when (coreType) {
is RustType.Vec -> renderVecHelper(member, memberName, coreType)
is RustType.HashMap -> renderMapHelper(member, memberName, coreType)
else -> renderBuilderMemberFn(this, coreType, member, memberName, memberSymbol)
else -> renderBuilderMemberFn(this, coreType, member, memberName)
}
renderBuilderMemberSetterFn(this, outerType, member, memberName, memberSymbol)
renderBuilderMemberSetterFn(this, outerType, member, memberName)
}
buildFn(this)
}

2
codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/FluentClientDecorator.kt
View File

@ -177,7 +177,7 @@ class GenericFluentClient(codegenContext: CodegenContext) : FluentClientCustomiz
/// - A retry policy (`R`) that dictates the behavior for requests that
/// fail and should (potentially) be retried. The default type is
/// generally what you want, as it implements a well-vetted retry
/// policy described in TODO.
/// policy implemented in [`retry::Standard`](crate::retry::Standard).
///
/// To construct a client, you will generally want to call
/// [`Client::with_config`], which takes a [`#{client}::Client`] (a

1
codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/config/ServiceConfigGenerator.kt
View File

@ -78,7 +78,6 @@ sealed class ServiceConfig(name: String) : Section(name) {
object BuilderBuild : ServiceConfig("BuilderBuild")
}
// TODO: if this becomes hot, it may need to be cached in a knowledge index
fun ServiceShape.needsIdempotencyToken(model: Model): Boolean {
val operationIndex = OperationIndex.of(model)
val topDownIndex = TopDownIndex.of(model)

2
codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/error/CombinedErrorGenerator.kt
View File

@ -151,8 +151,6 @@ class CombinedErrorGenerator(
}
}
// TODO: Consider if this should actually be `Option<Cow<&str>>`. This would enable us to use display
// as implemented by std::Error to generate a message in that case.
/// Returns the error message if one is available.
pub fn message(&self) -> Option<&str> {
self.meta.message()

2
codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/http/ResponseBindingGenerator.kt
View File

@ -303,7 +303,7 @@ class ResponseBindingGenerator(
rust("let $parsedValue = $parsedValue?;")
}
}
// TODO: this doesn't support non-optional vectors (which may be eventually added)
// TODO(https://github.com/awslabs/smithy-rs/issues/837): this doesn't support non-optional vectors
when (rustType) {
is RustType.Vec ->
rust(

7
codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/protocol/ProtocolGenerator.kt
View File

@ -236,10 +236,9 @@ open class ProtocolGenerator(
customizations: List<OperationCustomization>,
inputShape: StructureShape
) {
// TODO: One day, it should be possible for callers to invoke
// buildOperationType* directly to get the type rather than depending
// on these aliases.
// These are used in fluent clients
// TODO(https://github.com/awslabs/smithy-rs/issues/976): Callers should be able to invoke
// buildOperationType* directly to get the type rather than depending on these aliases.
// These are used in fluent clients.
val operationTypeOutput = buildOperationTypeOutput(inputWriter, operationShape)
val operationTypeRetry = buildOperationTypeRetry(inputWriter, customizations)
val inputPrefix = symbolProvider.toSymbol(inputShape).name

3
codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/testutil/Rust.kt
View File

@ -202,7 +202,6 @@ fun TestWriterDelegator.rustSettings(stubModel: Model) =
model = stubModel
)
// TODO: unify these test helpers a bit
fun String.shouldParseAsRust() {
// quick hack via rustfmt
val tempFile = File.createTempFile("rust_test", ".rs")
@ -219,7 +218,6 @@ fun RustWriter.compileAndTest(
clippy: Boolean = false,
expectFailure: Boolean = false
): String {
// TODO: if there are no dependencies, we can be a bit quicker
val deps = this.dependencies.map { RustDependency.fromSymbolDependency(it) }.filterIsInstance<CargoDependency>()
val module = if (this.namespace.contains("::")) {
this.namespace.split("::")[1]
@ -257,7 +255,6 @@ private fun String.intoCrate(
): File {
this.shouldParseAsRust()
val tempDir = TestWorkspace.subproject()
// TODO: unify this with CargoTomlGenerator
val cargoToml = """
[package]
name = ${tempDir.nameWithoutExtension.dq()}

4
rust-runtime/aws-smithy-http/src/property_bag.rs
View File

@ -43,10 +43,6 @@ impl Hasher for IdHasher {
///
/// `PropertyBag` can be used by `Request` and `Response` to store
/// data used to configure the SDK request pipeline.
///
/// TODO: We should consider if we want to require members of the property to be "resettable" in some
/// way to reset any state prior to a retry. I think this is worth delaying until we need it, but
/// is worth keeping in mind.
#[derive(Default)]
pub struct PropertyBag {
// In http where this property bag is usually empty, this makes sense. We will almost always put

1
tools/sdk-lints/Cargo.toml
View File

@ -2,6 +2,7 @@
name = "sdk-lints"
version = "0.1.0"
edition = "2018"
publish = false
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

67
tools/sdk-lints/src/changelog.rs
View File

@ -3,10 +3,12 @@
* SPDX-License-Identifier: Apache-2.0.
*/
use crate::lint::LintError;
use crate::{repo_root, Check, Lint};
use anyhow::{bail, Context, Result};
use serde::Deserialize;
use std::fmt::Write;
use std::path::Path;
use std::path::{Path, PathBuf};
use std::process::Command;
const EXAMPLE_ENTRY: &str = r#"
@ -83,7 +85,9 @@ fn indented_message(message: &str) -> String {
for (idx, line) in message.lines().enumerate() {
if idx > 0 {
out.push('\n');
out.push_str(" ");
if !line.is_empty() {
out.push_str(" ");
}
}
out.push_str(line);
}
@ -107,12 +111,6 @@ pub(crate) struct Changelog {
aws_sdk_rust: Vec<ChangelogEntry>,
}
impl Changelog {
pub(crate) fn num_entries(&self) -> usize {
self.smithy_rs.len() + self.aws_sdk_rust.len()
}
}
/// Ensure that there are no uncommited changes to the changelog
fn no_uncommited_changes(path: &Path) -> Result<()> {
let unstaged = !Command::new("git")
@ -145,7 +143,12 @@ pub(crate) fn update_changelogs(
no_uncommited_changes(changelog_next.as_ref()).context(
"CHANGELOG.next.toml had unstaged changes. Refusing to perform changelog update.",
)?;
let changelog = check_changelog_next(changelog_next.as_ref())?;
let changelog = check_changelog_next(changelog_next.as_ref()).map_err(|errs| {
anyhow::Error::msg(format!(
"cannot update changelogs with changelog errors: {:#?}",
errs
))
})?;
for (entries, path, version) in [
(changelog.smithy_rs, smithy_rs.as_ref(), smithy_rs_version),
(
@ -270,22 +273,44 @@ fn validate(entry: &ChangelogEntry) -> Result<()> {
Ok(())
}
/// Validate that `CHANGELOG.next.toml` follows best practices
pub(crate) fn check_changelog_next(path: impl AsRef<Path>) -> Result<Changelog> {
let contents = std::fs::read_to_string(path).context("failed to read CHANGELOG.next")?;
let parsed: Changelog = toml::from_str(&contents).context("Invalid changelog format")?;
let mut errors = 0;
for entry in parsed.aws_sdk_rust.iter().chain(parsed.smithy_rs.iter()) {
if let Err(e) = validate(entry) {
eprintln!("{:?}", e);
errors += 1;
pub(crate) struct ChangelogNext;
impl Lint for ChangelogNext {
fn name(&self) -> &str {
"Changelog.next"
}
fn files_to_check(&self) -> Result<Vec<PathBuf>> {
Ok(vec![repo_root().join("CHANGELOG.next.toml")])
}
}
impl Check for ChangelogNext {
fn check(&self, path: impl AsRef<Path>) -> Result<Vec<LintError>> {
match check_changelog_next(path) {
Ok(_) => Ok(vec![]),
Err(errs) => Ok(errs),
}
}
if errors == 0 {
eprintln!("Validated {} changelog entries", parsed.num_entries());
}
/// Validate that `CHANGELOG.next.toml` follows best practices
fn check_changelog_next(path: impl AsRef<Path>) -> std::result::Result<Changelog, Vec<LintError>> {
let contents = std::fs::read_to_string(path)
.context("failed to read CHANGELOG.next")
.map_err(|e| vec![LintError::via_display(e)])?;
let parsed: Changelog = toml::from_str(&contents)
.context("Invalid changelog format")
.map_err(|e| vec![LintError::via_display(e)])?;
let mut errors = vec![];
for entry in parsed.aws_sdk_rust.iter().chain(parsed.smithy_rs.iter()) {
if let Err(e) = validate(entry) {
errors.push(LintError::via_display(e))
}
}
if errors.is_empty() {
Ok(parsed)
} else {
bail!("Invalid changelog entries")
Err(errors)
}
}

40
tools/sdk-lints/src/copyright.rs
View File

@ -5,9 +5,11 @@
use std::ffi::OsStr;
use std::fs;
use std::path::Path;
use std::path::{Path, PathBuf};
use anyhow::{bail, Result};
use crate::lint::LintError;
use crate::{Check, Lint, VCS_FILES};
use anyhow::Result;
const EXPECTED_CONTENTS: &[&str] = &[
"Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.",
@ -16,22 +18,44 @@ const EXPECTED_CONTENTS: &[&str] = &[
const NEEDS_HEADER: [&str; 5] = ["sh", "py", "rs", "kt", "ts"];
pub(crate) fn check_copyright_header(path: impl AsRef<Path>) -> Result<()> {
/// Check that all files have the correct copyright header
pub(crate) struct CopyrightHeader;
impl Lint for CopyrightHeader {
fn name(&self) -> &str {
"Copyright header"
}
fn files_to_check(&self) -> Result<Vec<PathBuf>> {
Ok(VCS_FILES.clone())
}
}
impl Check for CopyrightHeader {
fn check(&self, path: impl AsRef<Path>) -> Result<Vec<LintError>> {
Ok(check_copyright_header(path))
}
}
fn check_copyright_header(path: impl AsRef<Path>) -> Vec<LintError> {
if !needs_copyright_header(path.as_ref()) {
return Ok(());
return vec![];
}
let contents = match fs::read_to_string(path.as_ref()) {
Ok(contents) => contents,
Err(err) if format!("{}", err).contains("No such file or directory") => {
eprintln!("Note: {} does not exist", path.as_ref().display());
return Ok(());
return vec![];
}
Err(e) => return Err(e)?,
Err(e) => return vec![LintError::via_display(e)],
};
if !has_copyright_header(&contents) {
bail!("{:?} is missing copyright header", path.as_ref())
return vec![LintError::new(format!(
"{} is missing copyright header",
path.as_ref().display()
))];
}
Ok(())
return vec![];
}
fn needs_copyright_header(path: &Path) -> bool {

141
tools/sdk-lints/src/lint.rs Normal file
View File

@ -0,0 +1,141 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0.
*/
use anyhow::Context;
use std::borrow::Cow;
use std::fmt::{Display, Formatter};
use std::path::{Path, PathBuf};
#[derive(Debug)]
pub struct LintError {
message: Cow<'static, str>,
context: Option<Cow<'static, str>>,
location: Option<PathBuf>,
}
impl LintError {
pub(crate) fn via_display<T: Display>(t: T) -> Self {
LintError::new(format!("{}", t))
}
pub(crate) fn new(message: impl Into<Cow<'static, str>>) -> Self {
LintError {
message: message.into(),
context: None,
location: None,
}
}
fn at_location(self, location: PathBuf) -> Self {
Self {
location: Some(location),
..self
}
}
}
impl Display for LintError {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.message)?;
if let Some(ctx) = &self.context {
write!(f, "({})", ctx)?;
}
if let Some(path) = &self.location {
write!(f, "({})", path.display())?;
}
Ok(())
}
}
pub(crate) trait Lint {
fn name(&self) -> &str;
fn files_to_check(&self) -> anyhow::Result<Vec<PathBuf>>;
fn check_all(&self) -> anyhow::Result<Vec<LintError>>
where
Self: Check,
{
let mut errors = vec![];
for path in self
.files_to_check()
.with_context(|| format!("failed to load file list for {}", self.name()))?
{
let new_errors = self
.check(&path)
.with_context(|| format!("error linting {}", &path.display()))?
.into_iter()
.map(|lint| lint.at_location(path.clone()));
errors.extend(new_errors);
}
if errors.len() == 0 {
eprintln!("{}...OK!", self.name())
} else {
eprintln!("Errors for {}:", self.name());
for error in &errors {
eprintln!(" {}", error)
}
}
Ok(errors)
}
fn fix_all(&self, dry_run: Mode) -> anyhow::Result<Vec<PathBuf>>
where
Self: Fix,
{
let mut fixes = vec![];
for path in self
.files_to_check()
.with_context(|| format!("failed to load file list for {}", self.name()))?
{
let (errs, new_content) = self
.fix(&path)
.with_context(|| format!("error attempting to fix {}", path.display()))?;
let current_content = std::fs::read_to_string(&path).unwrap_or_default();
if !errs.is_empty() {
eprintln!("Errors for {}:", path.display());
for error in &errs {
eprintln!(" {}", error)
}
}
if new_content != current_content {
if dry_run == Mode::NoDryRun {
std::fs::write(&path, new_content)
.with_context(|| format!("failure writing fix to {}", path.display()))?;
}
fixes.push(path);
}
}
if fixes.len() == 0 {
eprintln!("{}...OK!", self.name())
} else {
eprintln!("Fixed {} files for {}:", fixes.len(), self.name());
for file in &fixes {
eprintln!(" {}", file.display())
}
}
Ok(fixes)
}
}
pub(crate) trait Check: Lint {
fn check(&self, path: impl AsRef<Path>) -> anyhow::Result<Vec<LintError>>;
}
#[derive(Debug, Eq, PartialEq, Copy, Clone)]
pub(crate) enum Mode {
DryRun,
NoDryRun,
}
pub(crate) trait Fix: Lint {
fn fix(&self, path: impl AsRef<Path>) -> anyhow::Result<(Vec<LintError>, String)>;
}
impl<T> Check for T
where
T: Fix,
{
fn check(&self, path: impl AsRef<Path>) -> anyhow::Result<Vec<LintError>> {
self.fix(path).map(|(errs, _)| errs)
}
}

173
tools/sdk-lints/src/lint_cargo_toml.rs
View File

@ -4,12 +4,24 @@
*/
use crate::anchor::replace_anchor;
use anyhow::{bail, Context, Result};
use cargo_toml::Manifest;
use crate::lint::LintError;
use crate::{all_cargo_tomls, Check, Fix, Lint};
use anyhow::{Context, Result};
use cargo_toml::{Manifest, Package};
use serde::de::DeserializeOwned;
use serde::Deserialize;
use std::fs::read_to_string;
use std::ops::Deref;
use std::path::Path;
use std::path::{Path, PathBuf};
macro_rules! return_errors {
($errs: expr) => {
match $errs {
Ok(res) => res,
Err(errs) => return Ok(errs),
}
};
}
#[derive(Deserialize)]
struct DocsRsMetadata {
@ -48,19 +60,34 @@ const SERVER_CRATES: &[&str] = &["aws-smithy-http-server"];
/// Validates that:
/// - `[package.license]` is set to `Apache-2.0`
/// - A `LICENSE` file is present in the same directory as the `Cargo.toml` file
pub(crate) fn check_crate_license(path: impl AsRef<Path>) -> Result<()> {
let parsed = Manifest::from_path(path.as_ref()).context("failed to parse Cargo.toml")?;
let package = match parsed.package {
Some(package) => package,
None => bail!("missing `[package]` section"),
};
let license = match package.license {
Some(license) => license,
None => bail!("license must be specified"),
};
if license != "Apache-2.0" {
bail!("incorrect license: {}", license)
pub(crate) struct CrateLicense;
impl Lint for CrateLicense {
fn name(&self) -> &str {
"Crate licensing"
}
fn files_to_check(&self) -> Result<Vec<PathBuf>> {
Ok(all_cargo_tomls()?.collect())
}
}
impl Check for CrateLicense {
fn check(&self, path: impl AsRef<Path>) -> Result<Vec<LintError>> {
let package = return_errors!(package(&path)?);
check_crate_license(package, path)
}
}
fn check_crate_license(package: Package, path: impl AsRef<Path>) -> Result<Vec<LintError>> {
let mut errors = vec![];
match package.license {
Some(license) if license == "Apache-2.0" => {}
incorrect_license => errors.push(LintError::new(format!(
"invalid license: {:?}",
incorrect_license
))),
};
if !path
.as_ref()
.parent()
@ -68,30 +95,82 @@ pub(crate) fn check_crate_license(path: impl AsRef<Path>) -> Result<()> {
.join("LICENSE")
.exists()
{
bail!("LICENSE file missing")
errors.push(LintError::new("LICENSE file missing"));
}
Ok(())
Ok(errors)
}
pub(crate) fn check_crate_author(path: impl AsRef<Path>) -> Result<()> {
let parsed = Manifest::from_path(path).context("failed to parse Cargo.toml")?;
let package = match parsed.package {
Some(package) => package,
None => bail!("missing `[package]` section"),
};
/// Checks that the `author` field of the crate is either the Rust SDK team or the server SDK Team
pub(crate) struct CrateAuthor;
impl Lint for CrateAuthor {
fn name(&self) -> &str {
"Crate author"
}
fn files_to_check(&self) -> Result<Vec<PathBuf>> {
Ok(all_cargo_tomls()?.collect())
}
}
impl Check for CrateAuthor {
fn check(&self, path: impl AsRef<Path>) -> Result<Vec<LintError>> {
let package = return_errors!(package(path)?);
check_crate_author(package)
}
}
fn package<T>(path: impl AsRef<Path>) -> Result<std::result::Result<Package<T>, Vec<LintError>>>
where
T: DeserializeOwned,
{
let parsed = Manifest::from_path_with_metadata(path).context("failed to parse Cargo.toml")?;
match parsed.package {
Some(package) => Ok(Ok(package)),
None => return Ok(Err(vec![LintError::new("missing `[package]` section")])),
}
}
fn check_crate_author(package: Package) -> Result<Vec<LintError>> {
let mut errors = vec![];
let expected_author = if SERVER_CRATES.contains(&package.name.as_str()) {
SERVER_TEAM
} else {
RUST_SDK_TEAM
};
if !package.authors.iter().any(|s| s == expected_author) {
bail!(
errors.push(LintError::new(format!(
"missing `{}` in package author list ({:?})",
expected_author,
package.authors
)
expected_author, package.authors
)));
}
Ok(errors)
}
pub(crate) struct DocsRs;
impl Lint for DocsRs {
fn name(&self) -> &str {
"Cargo.toml package.metadata.docs.rs"
}
fn files_to_check(&self) -> Result<Vec<PathBuf>> {
Ok(all_cargo_tomls()?.collect())
}
}
impl Fix for DocsRs {
fn fix(&self, path: impl AsRef<Path>) -> Result<(Vec<LintError>, String)> {
let contents = read_to_string(&path)?;
let updated = fix_docs_rs(&contents)?;
let package = match package(path) {
Ok(Ok(package)) => package,
Ok(Err(errs)) => return Ok((errs, updated)),
Err(errs) => return Ok((vec![LintError::new(format!("{}", errs))], updated)),
};
let lint_errors = check_docs_rs(&package);
Ok((lint_errors, updated))
}
Ok(())
}
/// Check Cargo.toml for a valid docs.rs metadata section
@ -100,27 +179,26 @@ pub(crate) fn check_crate_author(path: impl AsRef<Path>) -> Result<()> {
/// - it is valid TOML
/// - it contains a package.metadata.docs.rs section
/// - All of the standard docs.rs settings are respected
pub(crate) fn check_docs_rs(path: impl AsRef<Path>) -> Result<()> {
let parsed = Manifest::<DocsRsMetadata>::from_path_with_metadata(path)
.context("failed to parse Cargo.toml")?;
let package = match parsed.package {
Some(package) => package,
None => bail!("missing `[package]` section"),
};
let metadata = match package.metadata {
fn check_docs_rs(package: &Package<DocsRsMetadata>) -> Vec<LintError> {
let metadata = match &package.metadata {
Some(metadata) => metadata,
None => bail!("mising `[docs.rs.metadata]` section"),
None => return vec![LintError::new("missing docs.rs metadata section")],
};
let mut errs = vec![];
if !metadata.all_features {
bail!("all-features must be set to true")
errs.push(LintError::new("all-features must be set to true"))
}
if metadata.targets != ["x86_64-unknown-linux-gnu"] {
bail!("targets must be pruned to linux only `x86_64-unknown-linux-gnu`")
errs.push(LintError::new(
"targets must be pruned to linux only `x86_64-unknown-linux-gnu`",
))
}
if metadata.rustdoc_args != ["--cfg", "docsrs"] {
bail!("rustdoc args must be [\"--cfg\", \"docsrs\"]");
errs.push(LintError::new(
"rustdoc args must be [\"--cfg\", \"docsrs\"]",
));
}
Ok(())
errs
}
const DEFAULT_DOCS_RS_SECTION: &str = r#"
@ -133,15 +211,12 @@ rustdoc-args = ["--cfg", "docsrs"]
///
/// `[package.metadata.docs.rs]` is used as the head anchor. A comment `# End of docs.rs metadata` is used
/// as the tail anchor.
pub(crate) fn fix_docs_rs(path: impl AsRef<Path>) -> Result<bool> {
let mut cargo_toml = read_to_string(path.as_ref()).context("failed to read Cargo.toml")?;
let updated = replace_anchor(
&mut cargo_toml,
fn fix_docs_rs(contents: &str) -> Result<String> {
let mut new = contents.to_string();
replace_anchor(
&mut new,
&("[package.metadata.docs.rs]", "# End of docs.rs metadata"),
DEFAULT_DOCS_RS_SECTION,
)?;
if updated {
std::fs::write(path.as_ref(), cargo_toml)?;
}
Ok(updated)
Ok(new)
}

190
tools/sdk-lints/src/main.rs
View File

@ -3,8 +3,12 @@
* SPDX-License-Identifier: Apache-2.0.
*/
use crate::changelog::check_changelog_next;
use crate::lint_cargo_toml::{check_crate_author, check_crate_license, check_docs_rs, fix_docs_rs};
use crate::changelog::ChangelogNext;
use crate::copyright::CopyrightHeader;
use crate::lint::{Check, Fix, Lint, LintError, Mode};
use crate::lint_cargo_toml::{CrateAuthor, CrateLicense, DocsRs};
use crate::readmes::{ReadmesExist, ReadmesHaveFooters};
use crate::todos::TodosHaveContext;
use anyhow::{bail, Context, Result};
use lazy_static::lazy_static;
use std::env::set_current_dir;
@ -16,7 +20,10 @@ use structopt::StructOpt;
mod anchor;
mod changelog;
mod copyright;
mod lint;
mod lint_cargo_toml;
mod readmes;
mod todos;
fn load_repo_root() -> Result<PathBuf> {
let output = Command::new("git")
@ -42,6 +49,8 @@ enum Args {
changelog: bool,
#[structopt(long)]
license: bool,
#[structopt(long)]
todos: bool,
},
Fix {
#[structopt(long)]
@ -50,6 +59,8 @@ enum Args {
docsrs_metadata: bool,
#[structopt(long)]
all: bool,
#[structopt(long)]
dry_run: Option<bool>,
},
UpdateChangelog {
#[structopt(long)]
@ -91,6 +102,14 @@ fn repo_root() -> &'static Path {
REPO_ROOT.as_path()
}
fn ok<T>(errors: Vec<T>) -> anyhow::Result<()> {
if errors.is_empty() {
Ok(())
} else {
bail!("Lint errors occurred");
}
}
fn main() -> Result<()> {
set_current_dir(repo_root())?;
let opt = Args::from_args();
@ -102,36 +121,48 @@ fn main() -> Result<()> {
docsrs_metadata,
changelog,
license,
todos,
} => {
let mut errs = vec![];
if readme || all {
check_readmes()?;
errs.extend(ReadmesExist.check_all()?);
errs.extend(ReadmesHaveFooters.check_all()?);
}
if cargo_toml || all {
check_authors()?;
check_license()?;
errs.extend(CrateAuthor.check_all()?);
errs.extend(CrateLicense.check_all()?);
}
if docsrs_metadata || all {
check_docsrs_metadata()?;
errs.extend(DocsRs.check_all()?);
}
if license || all {
check_license_header()?;
errs.extend(CopyrightHeader.check_all()?);
}
if changelog || all {
check_changelog_next(repo_root().join("CHANGELOG.next.toml"))?;
errs.extend(ChangelogNext.check_all()?);
}
if todos || all {
errs.extend(TodosHaveContext.check_all()?);
}
ok(errs)?
}
Args::Fix {
readme,
docsrs_metadata,
all,
dry_run,
} => {
let dry_run = match dry_run.unwrap_or(false) {
true => Mode::DryRun,
false => Mode::NoDryRun,
};
if readme || all {
fix_readmes()?
ok(ReadmesHaveFooters.fix_all(dry_run)?)?;
}
if docsrs_metadata || all {
fix_docs_rs_metadata()?
ok(DocsRs.fix_all(dry_run)?)?;
}
}
Args::UpdateChangelog {
@ -179,142 +210,3 @@ fn all_runtime_crates() -> Result<impl Iterator<Item = PathBuf>> {
fn all_cargo_tomls() -> Result<impl Iterator<Item = PathBuf>> {
Ok(all_runtime_crates()?.map(|pkg| pkg.join("Cargo.toml")))
}
fn check_authors() -> Result<()> {
let mut failed = 0;
for toml in all_cargo_tomls()? {
let local_path = toml.strip_prefix(repo_root()).expect("relative to root");
let result = check_crate_author(toml.as_path())
.with_context(|| format!("Error in {:?}", local_path));
if let Err(e) = result {
failed += 1;
eprintln!("{:?}", e);
}
}
if failed > 0 {
bail!("{} crates had incorrect crate authors", failed)
} else {
eprintln!("All crates had correct authorship!");
Ok(())
}
}
fn check_license_header() -> Result<()> {
let mut failed = 0;
for license in VCS_FILES.iter() {
let result = copyright::check_copyright_header(license);
if let Err(e) = result {
failed += 1;
eprintln!("{:?}", e);
}
}
if failed > 0 {
bail!("{} files missing license headers", failed)
}
eprintln!("All files had correct license headers");
Ok(())
}
/// Check that all crates have correct licensing
fn check_license() -> Result<()> {
let mut failed = 0;
for toml in all_cargo_tomls()? {
let local_path = toml.strip_prefix(repo_root()).expect("relative to root");
let result = check_crate_license(toml.as_path())
.with_context(|| format!("Error in {:?}", local_path));
if let Err(e) = result {
failed += 1;
eprintln!("{:?}", e);
}
}
if failed > 0 {
bail!("{} crates had incorrect crate licenses", failed)
} else {
eprintln!("All crates had correct licenses!");
Ok(())
}
}
/// Check that all crates have correct `[package.metadata.docs.rs]` settings
fn check_docsrs_metadata() -> Result<()> {
let mut failed = 0;
for toml in all_cargo_tomls()? {
let local_path = toml.strip_prefix(repo_root()).expect("relative to root");
let result =
check_docs_rs(toml.as_path()).with_context(|| format!("Error in {:?}", local_path));
if let Err(e) = result {
failed += 1;
eprintln!("{:?}", e);
}
}
if failed > 0 {
bail!("{} crates had incorrect docsrs metadata", failed)
} else {
eprintln!("All crates had correct docsrs metadata!");
Ok(())
}
}
/// Checks that all crates have README files
fn check_readmes() -> Result<()> {
let no_readme = all_runtime_crates()?.filter(|dir| !dir.join("README.md").exists());
let mut failed = 0;
for bad_crate in no_readme {
eprintln!(
"{:?} is missing a README",
bad_crate
.strip_prefix(repo_root())
.expect("must be relative to repo root")
);
failed += 1;
}
if failed > 0 {
bail!("{} crates were missing READMEs", failed)
} else {
eprintln!("All crates have READMEs!");
Ok(())
}
}
fn fix_readmes() -> Result<()> {
let readmes = all_runtime_crates()?.map(|pkg| pkg.join("README.md"));
let mut num_fixed = 0;
for readme in readmes {
num_fixed += fix_readme(readme)?.then(|| 1).unwrap_or_default();
}
if num_fixed > 0 {
bail!("Updated {} READMEs with footer.", num_fixed);
} else {
eprintln!("All READMEs have correct footers");
Ok(())
}
}
fn fix_docs_rs_metadata() -> Result<()> {
let cargo_tomls = all_cargo_tomls()?;
let mut num_fixed = 0;
for cargo_toml in cargo_tomls {
num_fixed += fix_docs_rs(cargo_toml.as_path())
.with_context(|| format!("{:?}", cargo_toml))?
.then(|| 1)
.unwrap_or_default();
}
if num_fixed > 0 {
bail!("Updated {} metadata files", num_fixed);
} else {
eprintln!("All crates have correct metadata");
Ok(())
}
}
const README_FOOTER: &str = "\nThis crate is part of the [AWS SDK for Rust](https://awslabs.github.io/aws-sdk-rust/) \
and the [smithy-rs](https://github.com/awslabs/smithy-rs) code generator. In most cases, it should not be used directly.\n";
fn fix_readme(path: impl AsRef<Path>) -> Result<bool> {
let mut contents = fs::read_to_string(path.as_ref())
.with_context(|| format!("failure to read readme: {:?}", path.as_ref()))?;
let updated = anchor::replace_anchor(&mut contents, &anchor::anchors("footer"), README_FOOTER)?;
fs::write(path.as_ref(), contents)?;
Ok(updated)
}

63
tools/sdk-lints/src/readmes.rs Normal file
View File

@ -0,0 +1,63 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0.
*/
use crate::{all_runtime_crates, anchor, Check, Fix, Lint, LintError};
use anyhow::{Context, Result};
use std::fs;
use std::path::{Path, PathBuf};
pub(crate) struct ReadmesExist;
impl Lint for ReadmesExist {
fn name(&self) -> &str {
"Crates have readmes"
}
fn files_to_check(&self) -> Result<Vec<PathBuf>> {
Ok(all_runtime_crates()?.collect())
}
}
impl Check for ReadmesExist {
fn check(&self, path: impl AsRef<Path>) -> Result<Vec<crate::lint::LintError>> {
if !path.as_ref().join("README.md").exists() {
return Ok(vec![LintError::new("Crate is missing a README")]);
}
return Ok(vec![]);
}
}
pub(crate) struct ReadmesHaveFooters;
impl Lint for ReadmesHaveFooters {
fn name(&self) -> &str {
"READMEs have footers"
}
fn files_to_check(&self) -> Result<Vec<PathBuf>> {
Ok(all_runtime_crates()?
.map(|path| path.join("README.md"))
.collect())
}
}
impl Fix for ReadmesHaveFooters {
fn fix(&self, path: impl AsRef<Path>) -> Result<(Vec<LintError>, String)> {
let (updated, new_contents) = fix_readme(path)?;
let errs = match updated {
true => vec![LintError::new("README was missing required footer")],
false => vec![],
};
Ok((errs, new_contents))
}
}
const README_FOOTER: &str = "\nThis crate is part of the [AWS SDK for Rust](https://awslabs.github.io/aws-sdk-rust/) \
and the [smithy-rs](https://github.com/awslabs/smithy-rs) code generator. In most cases, it should not be used directly.\n";
fn fix_readme(path: impl AsRef<Path>) -> Result<(bool, String)> {
let mut contents = fs::read_to_string(path.as_ref())
.with_context(|| format!("failure to read readme: {:?}", path.as_ref()))?;
let updated = anchor::replace_anchor(&mut contents, &anchor::anchors("footer"), README_FOOTER)?;
Ok((updated, contents))
}

67
tools/sdk-lints/src/todos.rs Normal file
View File

@ -0,0 +1,67 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0.
*/
use crate::lint::LintError;
use crate::{Check, Lint, VCS_FILES};
use std::ffi::OsStr;
use std::fs;
use std::path::{Path, PathBuf};
// All "TODOs" must include (...) that gives them context
pub(crate) struct TodosHaveContext;
const IGNORE_DIRS: &[&str] = &[
"codegen-server",
"codegen-server-test",
"rust-runtime/aws-smithy-http-server",
"tools/sdk-lints/src/todos.rs",
];
impl Lint for TodosHaveContext {
fn name(&self) -> &str {
"TODOs include context"
}
fn files_to_check(&self) -> anyhow::Result<Vec<PathBuf>> {
fn validate_todos(extension: Option<&OsStr>) -> bool {
extension
.map(|ext| {
ext.eq_ignore_ascii_case("rs")
|| ext.eq_ignore_ascii_case("kt")
|| ext.eq_ignore_ascii_case("kts")
})
.unwrap_or(false)
}
Ok(VCS_FILES
.iter()
.filter(|path| !IGNORE_DIRS.iter().any(|dir| path.starts_with(dir)))
.filter(|f| validate_todos(f.extension()))
.map(|t| t.clone())
.collect())
}
}
impl Check for TodosHaveContext {
fn check(&self, path: impl AsRef<Path>) -> anyhow::Result<Vec<LintError>> {
let contents = match fs::read_to_string(path.as_ref()) {
Ok(contents) => contents,
Err(err) if format!("{}", err).contains("No such file or directory") => {
eprintln!("Note: {} does not exist", path.as_ref().display());
return Ok(vec![]);
}
e @ Err(_) => e?,
};
let mut errs = vec![];
for todo in contents.split("TODO").skip(1) {
if !todo.starts_with('(') {
let todo_line = todo.lines().next().unwrap_or_default();
errs.push(LintError::new(format!(
"TODO without context: `TODO{}`",
todo_line
)))
}
}
Ok(errs)
}
}