From a647f31600532fdc8fca65f2b0e5be4f1de68b98 Mon Sep 17 00:00:00 2001 From: konsti Date: Mon, 3 Jul 2023 21:48:44 +0200 Subject: [PATCH 01/15] Don't add a magic trailing comma for a single entry (#5463) ## Summary If a comma separated list has only one entry, black will respect the magic trailing comma, but it will not add a new one. The following code will remain as is: ```python b1 = [ aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa ] b2 = [ aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, ] b3 = [ aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa ] ``` ## Test Plan This was first discovered in https://github.com/django/django/blob/7eeadc82c2f7d7a778e3bb43c34d642e6275dacf/django/contrib/admin/checks.py#L674-L681, which i've minimized into a call test. I've added tests for the three cases (one entry + no comma, one entry + comma, more than one entry) to the list tests. The diffs from the black tests get smaller. --- .../ruff/{statement => expression}/call.py | 5 ++++ .../test/fixtures/ruff/expression/list.py | 13 ++++++++ crates/ruff_python_formatter/src/builders.rs | 30 ++++++++++++------- ...aneous__long_strings_flag_disabled.py.snap | 29 +++++------------- ...ity@py_310__pattern_matching_style.py.snap | 12 ++++---- ...patibility@simple_cases__comments3.py.snap | 11 +------ ...tibility@simple_cases__composition.py.snap | 11 +------ ...ses__composition_no_trailing_comma.py.snap | 11 +------ ...mpatibility@simple_cases__fmtonoff.py.snap | 10 ++----- ...patibility@simple_cases__fmtonoff4.py.snap | 15 ++-------- ...patibility@simple_cases__fmtonoff5.py.snap | 4 +-- ...mpatibility@simple_cases__function.py.snap | 28 ++++------------- ...ple_cases__function_trailing_comma.py.snap | 30 ++++--------------- .../format@expression__binary.py.snap | 4 +-- ...y.snap => format@expression__call.py.snap} | 17 +++++++++-- .../format@expression__compare.py.snap | 4 +-- .../snapshots/format@expression__dict.py.snap | 4 +-- .../snapshots/format@expression__list.py.snap | 26 ++++++++++++++++ .../snapshots/format@statement__with.py.snap | 4 +-- 19 files changed, 119 insertions(+), 149 deletions(-) rename crates/ruff_python_formatter/resources/test/fixtures/ruff/{statement => expression}/call.py (85%) rename crates/ruff_python_formatter/tests/snapshots/{format@statement__call.py.snap => format@expression__call.py.snap} (83%) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/call.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/call.py similarity index 85% rename from crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/call.py rename to crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/call.py index 7a32b6cd28970..8c372180ce7f5 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/call.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/call.py @@ -81,3 +81,8 @@ def f(*args, **kwargs): dict() ) +# Don't add a magic trailing comma when there is only one entry +# Minimized from https://github.com/django/django/blob/7eeadc82c2f7d7a778e3bb43c34d642e6275dacf/django/contrib/admin/checks.py#L674-L681 +f( + a.very_long_function_function_that_is_so_long_that_it_expands_the_parent_but_its_only_a_single_argument() +) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/list.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/list.py index 2ec1c0e293028..f0fedc6957da1 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/list.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/list.py @@ -8,3 +8,16 @@ a3 = [ # b ] + +# Add magic trailing comma only if there is more than one entry, but respect it if it's +# already there +b1 = [ + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa +] +b2 = [ + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, +] +b3 = [ + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa +] diff --git a/crates/ruff_python_formatter/src/builders.rs b/crates/ruff_python_formatter/src/builders.rs index 97a70dbe47e82..4541acda740ee 100644 --- a/crates/ruff_python_formatter/src/builders.rs +++ b/crates/ruff_python_formatter/src/builders.rs @@ -182,7 +182,10 @@ impl<'fmt, 'ast, 'buf> JoinNodesBuilder<'fmt, 'ast, 'buf> { pub(crate) struct JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> { result: FormatResult<()>, fmt: &'fmt mut PyFormatter<'ast, 'buf>, - last_end: Option, + end_of_last_entry: Option, + /// We need to track whether we have more than one entry since a sole entry doesn't get a + /// magic trailing comma even when expanded + len: usize, } impl<'fmt, 'ast, 'buf> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> { @@ -190,7 +193,8 @@ impl<'fmt, 'ast, 'buf> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> { Self { fmt: f, result: Ok(()), - last_end: None, + end_of_last_entry: None, + len: 0, } } @@ -203,11 +207,12 @@ impl<'fmt, 'ast, 'buf> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> { T: Ranged, { self.result = self.result.and_then(|_| { - if self.last_end.is_some() { + if self.end_of_last_entry.is_some() { write!(self.fmt, [text(","), soft_line_break_or_space()])?; } - self.last_end = Some(node.end()); + self.end_of_last_entry = Some(node.end()); + self.len += 1; content.fmt(self.fmt) }); @@ -243,18 +248,23 @@ impl<'fmt, 'ast, 'buf> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> { pub(crate) fn finish(&mut self) -> FormatResult<()> { self.result.and_then(|_| { - if let Some(last_end) = self.last_end.take() { - if_group_breaks(&text(",")).fmt(self.fmt)?; - - if self.fmt.options().magic_trailing_comma().is_respect() + if let Some(last_end) = self.end_of_last_entry.take() { + let magic_trailing_comma = self.fmt.options().magic_trailing_comma().is_respect() && matches!( first_non_trivia_token(last_end, self.fmt.context().contents()), Some(Token { kind: TokenKind::Comma, .. }) - ) - { + ); + + // If there is a single entry, only keep the magic trailing comma, don't add it if + // it wasn't there. If there is more than one entry, always add it. + if magic_trailing_comma || self.len > 1 { + if_group_breaks(&text(",")).fmt(self.fmt)?; + } + + if magic_trailing_comma { expand_parent().fmt(self.fmt)?; } } diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@miscellaneous__long_strings_flag_disabled.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@miscellaneous__long_strings_flag_disabled.py.snap index e969e1e7a7fc0..a3f36ae2e66ec 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@miscellaneous__long_strings_flag_disabled.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@miscellaneous__long_strings_flag_disabled.py.snap @@ -312,15 +312,6 @@ long_unmergable_string_with_pragma = ( y = "Short string" -@@ -12,7 +12,7 @@ - ) - - print( -- "This is a really long string inside of a print statement with no extra arguments attached at the end of it." -+ "This is a really long string inside of a print statement with no extra arguments attached at the end of it.", - ) - - D1 = { @@ -70,8 +70,8 @@ bad_split3 = ( "What if we have inline comments on " # First Comment @@ -367,7 +358,7 @@ long_unmergable_string_with_pragma = ( comment_string = "Long lines with inline comments should have their comments appended to the reformatted string's enclosing right parentheses." # This comment gets thrown to the top. -@@ -165,30 +163,18 @@ +@@ -165,25 +163,13 @@ triple_quote_string = """This is a really really really long triple quote string assignment and it should not be touched.""" @@ -397,12 +388,6 @@ long_unmergable_string_with_pragma = ( some_function_call( "With a reallly generic name and with a really really long string that is, at some point down the line, " - + added -- + " to a variable and then added to another string." -+ + " to a variable and then added to another string.", - ) - - some_function_call( @@ -212,29 +198,25 @@ ) @@ -412,7 +397,7 @@ long_unmergable_string_with_pragma = ( - " which should NOT be there." - ), + "This is a really long string argument to a function that has a trailing comma" -+ " which should NOT be there.", ++ " which should NOT be there." ) func_with_bad_comma( @@ -421,7 +406,7 @@ long_unmergable_string_with_pragma = ( - " which should NOT be there." - ), # comment after comma + "This is a really long string argument to a function that has a trailing comma" -+ " which should NOT be there.", # comment after comma ++ " which should NOT be there." # comment after comma ) func_with_bad_parens_that_wont_fit_in_one_line( @@ -498,7 +483,7 @@ print( ) print( - "This is a really long string inside of a print statement with no extra arguments attached at the end of it.", + "This is a really long string inside of a print statement with no extra arguments attached at the end of it." ) D1 = { @@ -660,7 +645,7 @@ NOT_YET_IMPLEMENTED_StmtAssert some_function_call( "With a reallly generic name and with a really really long string that is, at some point down the line, " + added - + " to a variable and then added to another string.", + + " to a variable and then added to another string." ) some_function_call( @@ -685,12 +670,12 @@ func_with_bad_comma( func_with_bad_comma( "This is a really long string argument to a function that has a trailing comma" - " which should NOT be there.", + " which should NOT be there." ) func_with_bad_comma( "This is a really long string argument to a function that has a trailing comma" - " which should NOT be there.", # comment after comma + " which should NOT be there." # comment after comma ) func_with_bad_parens_that_wont_fit_in_one_line( diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_310__pattern_matching_style.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_310__pattern_matching_style.py.snap index 6a8135be42936..56fe93fb726bf 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_310__pattern_matching_style.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_310__pattern_matching_style.py.snap @@ -84,7 +84,7 @@ match match( -match(arg) # comment +match( -+ arg, # comment ++ arg # comment +) match() @@ -93,7 +93,7 @@ match match( -case(arg) # comment +case( -+ arg, # comment ++ arg # comment +) case() @@ -103,7 +103,7 @@ match match( -re.match(something) # fast +re.match( -+ something, # fast ++ something # fast +) re.match() -match match(): @@ -120,7 +120,7 @@ match match( NOT_YET_IMPLEMENTED_StmtMatch match( - arg, # comment + arg # comment ) match() @@ -128,7 +128,7 @@ match() match() case( - arg, # comment + arg # comment ) case() @@ -137,7 +137,7 @@ case() re.match( - something, # fast + something # fast ) re.match() NOT_YET_IMPLEMENTED_StmtMatch diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments3.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments3.py.snap index a79cf2b66b54d..2812b9d37a68c 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments3.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments3.py.snap @@ -76,15 +76,6 @@ def func(): # Capture each of the exceptions in the MultiError along with each of their causes and contexts if isinstance(exc_value, MultiError): embedded = [] -@@ -29,7 +22,7 @@ - # copy the set of _seen exceptions so that duplicates - # shared between sub-exceptions are not omitted - _seen=set(_seen), -- ) -+ ), - # This should be left alone (after) - ) - ``` ## Ruff Output @@ -114,7 +105,7 @@ def func(): # copy the set of _seen exceptions so that duplicates # shared between sub-exceptions are not omitted _seen=set(_seen), - ), + ) # This should be left alone (after) ) diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition.py.snap index deda69dc2e1d7..399e4ab91541f 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition.py.snap @@ -203,15 +203,6 @@ class C: print(i) xxxxxxxxxxxxxxxx = Yyyy2YyyyyYyyyyy( push_manager=context.request.resource_manager, -@@ -37,7 +37,7 @@ - batch_size=Yyyy2YyyyYyyyyYyyy.FULL_SIZE, - ).push( - # Only send the first n items. -- items=items[:num_items] -+ items=items[:num_items], - ) - return ( - 'Utterly failed doctest test for %s\n File "%s", line %s, in %s\n\n%s' @@ -47,113 +47,46 @@ def omitting_trailers(self) -> None: get_collection( @@ -418,7 +409,7 @@ class C: batch_size=Yyyy2YyyyYyyyyYyyy.FULL_SIZE, ).push( # Only send the first n items. - items=items[:num_items], + items=items[:num_items] ) return ( 'Utterly failed doctest test for %s\n File "%s", line %s, in %s\n\n%s' diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition_no_trailing_comma.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition_no_trailing_comma.py.snap index 993bab559a158..69415159cec31 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition_no_trailing_comma.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__composition_no_trailing_comma.py.snap @@ -203,15 +203,6 @@ class C: print(i) xxxxxxxxxxxxxxxx = Yyyy2YyyyyYyyyyy( push_manager=context.request.resource_manager, -@@ -37,7 +37,7 @@ - batch_size=Yyyy2YyyyYyyyyYyyy.FULL_SIZE, - ).push( - # Only send the first n items. -- items=items[:num_items] -+ items=items[:num_items], - ) - return ( - 'Utterly failed doctest test for %s\n File "%s", line %s, in %s\n\n%s' @@ -47,113 +47,46 @@ def omitting_trailers(self) -> None: get_collection( @@ -418,7 +409,7 @@ class C: batch_size=Yyyy2YyyyYyyyyYyyy.FULL_SIZE, ).push( # Only send the first n items. - items=items[:num_items], + items=items[:num_items] ) return ( 'Utterly failed doctest test for %s\n File "%s", line %s, in %s\n\n%s' diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff.py.snap index 4d40d39ee77e5..32082f2da9457 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff.py.snap @@ -395,12 +395,8 @@ d={'a':1, # fmt: on # fmt: off # ...but comments still get reformatted even though they should not be -@@ -150,12 +172,10 @@ - ast_args.kw_defaults, - parameters, - implicit_default=True, -- ) -+ ), +@@ -153,9 +175,7 @@ + ) ) # fmt: off - a = ( @@ -610,7 +606,7 @@ def long_lines(): ast_args.kw_defaults, parameters, implicit_default=True, - ), + ) ) # fmt: off a = unnecessary_bracket() diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff4.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff4.py.snap index 89b2436af159b..a8dc2ef62014f 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff4.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff4.py.snap @@ -37,20 +37,11 @@ def f(): pass + 2, + 3, + 4, -+ ], ++ ] +) # fmt: on def f(): pass -@@ -14,7 +18,7 @@ - 2, - 3, - 4, -- ] -+ ], - ) - def f(): - pass ``` ## Ruff Output @@ -63,7 +54,7 @@ def f(): pass 2, 3, 4, - ], + ] ) # fmt: on def f(): @@ -76,7 +67,7 @@ def f(): 2, 3, 4, - ], + ] ) def f(): pass diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff5.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff5.py.snap index 5cc53343209b9..9cd5e7ed31e6e 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff5.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff5.py.snap @@ -103,7 +103,7 @@ elif unformatted: - # fmt: on - ] # Includes an formatted indentation. + # fmt: on -+ ], # Includes an formatted indentation. ++ ] # Includes an formatted indentation. }, ) @@ -186,7 +186,7 @@ setup( "foo-bar" "=foo.bar.:main", # fmt: on - ], # Includes an formatted indentation. + ] # Includes an formatted indentation. }, ) diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function.py.snap index eed13bbae7ecc..771b8b6398197 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function.py.snap @@ -111,14 +111,14 @@ def __await__(): return (yield) #!/usr/bin/env python3 -import asyncio -import sys -- --from third_party import X, Y, Z +NOT_YET_IMPLEMENTED_StmtImport +NOT_YET_IMPLEMENTED_StmtImport --from library import some_connection, some_decorator +-from third_party import X, Y, Z +NOT_YET_IMPLEMENTED_StmtImportFrom +-from library import some_connection, some_decorator +- -f"trigger 3.6 mode" +NOT_YET_IMPLEMENTED_StmtImportFrom +NOT_YET_IMPLEMENTED_ExprJoinedStr @@ -198,24 +198,6 @@ def __await__(): return (yield) def long_lines(): -@@ -87,7 +94,7 @@ - ast_args.kw_defaults, - parameters, - implicit_default=True, -- ) -+ ), - ) - typedargslist.extend( - gen_annotated_params( -@@ -96,7 +103,7 @@ - parameters, - implicit_default=True, - # trailing standalone comment -- ) -+ ), - ) - _type_comment_re = re.compile( - r""" @@ -135,14 +142,8 @@ a, **kwargs, @@ -334,7 +316,7 @@ def long_lines(): ast_args.kw_defaults, parameters, implicit_default=True, - ), + ) ) typedargslist.extend( gen_annotated_params( @@ -343,7 +325,7 @@ def long_lines(): parameters, implicit_default=True, # trailing standalone comment - ), + ) ) _type_comment_re = re.compile( r""" diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function_trailing_comma.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function_trailing_comma.py.snap index 747b512442738..fcb6ee100ba23 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function_trailing_comma.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function_trailing_comma.py.snap @@ -73,15 +73,6 @@ some_module.some_function( ```diff --- Black +++ Ruff -@@ -27,7 +27,7 @@ - call( - arg={ - "explode": "this", -- } -+ }, - ) - call2( - arg=[1, 2, 3], @@ -35,7 +35,9 @@ x = { "a": 1, @@ -93,7 +84,7 @@ some_module.some_function( if ( a == { -@@ -47,22 +49,24 @@ +@@ -47,14 +49,16 @@ "f": 6, "g": 7, "h": 8, @@ -114,17 +105,6 @@ some_module.some_function( json = { "k": { "k2": { - "k3": [ - 1, -- ] -- } -- } -+ ], -+ }, -+ }, - } - - @@ -80,18 +84,14 @@ pass @@ -182,7 +162,7 @@ def f( call( arg={ "explode": "this", - }, + } ) call2( arg=[1, 2, 3], @@ -219,9 +199,9 @@ def xxxxxxxxxxxxxxxxxxxxxxxxxxxx() -> Set[ "k2": { "k3": [ 1, - ], - }, - }, + ] + } + } } diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__binary.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__binary.py.snap index 4e4a09fe837d1..40a69332f5efe 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__binary.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__binary.py.snap @@ -299,9 +299,9 @@ not (aaaaaaaaaaaaaa + {NOT_IMPLEMENTED_set_value for value in NOT_IMPLEMENTED_se [ a + [ - bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ] - in c, + in c ] diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__call.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap similarity index 83% rename from crates/ruff_python_formatter/tests/snapshots/format@statement__call.py.snap rename to crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap index f59509126f500..408588813cb8b 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__call.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap @@ -1,6 +1,6 @@ --- source: crates/ruff_python_formatter/tests/fixtures.rs -input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/call.py +input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/call.py --- ## Input ```py @@ -87,6 +87,11 @@ f( dict() ) +# Don't add a magic trailing comma when there is only one entry +# Minimized from https://github.com/django/django/blob/7eeadc82c2f7d7a778e3bb43c34d642e6275dacf/django/contrib/admin/checks.py#L674-L681 +f( + a.very_long_function_function_that_is_so_long_that_it_expands_the_parent_but_its_only_a_single_argument() +) ``` ## Output @@ -111,10 +116,10 @@ f(x=2) f(1, x=2) f( - this_is_a_very_long_argument_asökdhflakjslaslhfdlaffahflahsöfdhasägporejfäalkdsjäfalisjäfdlkasjd, + this_is_a_very_long_argument_asökdhflakjslaslhfdlaffahflahsöfdhasägporejfäalkdsjäfalisjäfdlkasjd ) f( - this_is_a_very_long_keyword_argument_asökdhflakjslaslhfdlaffahflahsöfdhasägporejfäalkdsjäfalisjäfdlkasjd=1, + this_is_a_very_long_keyword_argument_asökdhflakjslaslhfdlaffahflahsöfdhasägporejfäalkdsjäfalisjäfdlkasjd=1 ) f( @@ -168,6 +173,12 @@ f( **dict(), # oddly placed own line comment ) + +# Don't add a magic trailing comma when there is only one entry +# Minimized from https://github.com/django/django/blob/7eeadc82c2f7d7a778e3bb43c34d642e6275dacf/django/contrib/admin/checks.py#L674-L681 +f( + a.very_long_function_function_that_is_so_long_that_it_expands_the_parent_but_its_only_a_single_argument() +) ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap index 6031fbadf8328..03d75d057bf06 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap @@ -180,10 +180,10 @@ return ( ( a + [ - bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ] >= c - ), + ) ] ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__dict.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__dict.py.snap index 25a854128c4a6..bfafb3e2e7549 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__dict.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__dict.py.snap @@ -69,7 +69,7 @@ a = { # before { # open - key: value, # key # colon # value + key: value # key # colon # value } # close # after @@ -82,7 +82,7 @@ a = { } { - **b, # middle with single item + **b # middle with single item } { diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__list.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__list.py.snap index 8930e0036c452..e21436a052668 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__list.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__list.py.snap @@ -14,6 +14,19 @@ a2 = [ # a a3 = [ # b ] + +# Add magic trailing comma only if there is more than one entry, but respect it if it's +# already there +b1 = [ + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa +] +b2 = [ + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, +] +b3 = [ + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa +] ``` ## Output @@ -28,6 +41,19 @@ a2 = [ # a a3 = [ # b ] + +# Add magic trailing comma only if there is more than one entry, but respect it if it's +# already there +b1 = [ + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa +] +b2 = [ + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, +] +b3 = [ + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, +] ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap index 0dd8743d483c0..52be9741ad47d 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap @@ -113,9 +113,7 @@ with a: # should remove brackets # if we do want to wrap, do we prefer to wrap the entire WithItem or to let the # WithItem allow the `aa + bb` content expression to be wrapped with ( - ( - aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb - ) as c, + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as c ): ... From 6acc316d19ad4a9d24a1ed46b6020b84c5a3c28a Mon Sep 17 00:00:00 2001 From: Aarni Koskela Date: Tue, 4 Jul 2023 02:35:16 +0300 Subject: [PATCH 02/15] Turn Linters', etc. implicit `into_iter()`s into explicit `rules()` (#5436) ## Summary As discussed on ~IRC~ Discord, this will make it easier for e.g. the docs generation stuff to get all rules for a linter (using `all_rules()`) instead of just non-nursery ones, and it also makes it more Explicit Is Better Than Implicit to iterate over linter rules. Grepping for `Item = Rule` reveals some remaining implicit `IntoIterator`s that I didn't feel were necessarily in scope for this (and honestly, iterating over a `RuleSet` makes sense). --- crates/ruff/src/flake8_to_ruff/plugin.rs | 2 +- crates/ruff/src/registry.rs | 2 +- crates/ruff/src/rule_selector.rs | 14 ++-- .../src/rules/flake8_type_checking/mod.rs | 2 +- crates/ruff/src/rules/pandas_vet/mod.rs | 6 +- crates/ruff/src/rules/pyflakes/mod.rs | 4 +- crates/ruff_dev/src/generate_rules_table.rs | 4 +- crates/ruff_macros/src/map_codes.rs | 68 +++++++++---------- 8 files changed, 50 insertions(+), 52 deletions(-) diff --git a/crates/ruff/src/flake8_to_ruff/plugin.rs b/crates/ruff/src/flake8_to_ruff/plugin.rs index 77f6645d29a12..c234556c8ada5 100644 --- a/crates/ruff/src/flake8_to_ruff/plugin.rs +++ b/crates/ruff/src/flake8_to_ruff/plugin.rs @@ -333,7 +333,7 @@ pub(crate) fn infer_plugins_from_codes(selectors: &HashSet) -> Vec for selector in selectors { if selector .into_iter() - .any(|rule| Linter::from(plugin).into_iter().any(|r| r == rule)) + .any(|rule| Linter::from(plugin).rules().any(|r| r == rule)) { return true; } diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index dd47f24815cd6..d53cd6ac7e847 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -19,7 +19,7 @@ impl Rule { pub fn from_code(code: &str) -> Result { let (linter, code) = Linter::parse_code(code).ok_or(FromCodeError::Unknown)?; let prefix: RuleCodePrefix = RuleCodePrefix::parse(&linter, code)?; - Ok(prefix.into_iter().next().unwrap()) + Ok(prefix.rules().next().unwrap()) } } diff --git a/crates/ruff/src/rule_selector.rs b/crates/ruff/src/rule_selector.rs index 6247346a51da1..5eb5f1461b7bd 100644 --- a/crates/ruff/src/rule_selector.rs +++ b/crates/ruff/src/rule_selector.rs @@ -158,16 +158,16 @@ impl IntoIterator for &RuleSelector { } RuleSelector::C => RuleSelectorIter::Chain( Linter::Flake8Comprehensions - .into_iter() - .chain(Linter::McCabe.into_iter()), + .rules() + .chain(Linter::McCabe.rules()), ), RuleSelector::T => RuleSelectorIter::Chain( Linter::Flake8Debugger - .into_iter() - .chain(Linter::Flake8Print.into_iter()), + .rules() + .chain(Linter::Flake8Print.rules()), ), - RuleSelector::Linter(linter) => RuleSelectorIter::Vec(linter.into_iter()), - RuleSelector::Prefix { prefix, .. } => RuleSelectorIter::Vec(prefix.into_iter()), + RuleSelector::Linter(linter) => RuleSelectorIter::Vec(linter.rules()), + RuleSelector::Prefix { prefix, .. } => RuleSelectorIter::Vec(prefix.clone().rules()), } } } @@ -346,7 +346,7 @@ mod clap_completion { let prefix = p.linter().common_prefix(); let code = p.short_code(); - let mut rules_iter = p.into_iter(); + let mut rules_iter = p.rules(); let rule1 = rules_iter.next(); let rule2 = rules_iter.next(); diff --git a/crates/ruff/src/rules/flake8_type_checking/mod.rs b/crates/ruff/src/rules/flake8_type_checking/mod.rs index 06ea6ab839a0c..01aa8c60e4953 100644 --- a/crates/ruff/src/rules/flake8_type_checking/mod.rs +++ b/crates/ruff/src/rules/flake8_type_checking/mod.rs @@ -327,7 +327,7 @@ mod tests { fn contents(contents: &str, snapshot: &str) { let diagnostics = test_snippet( contents, - &settings::Settings::for_rules(&Linter::Flake8TypeChecking), + &settings::Settings::for_rules(Linter::Flake8TypeChecking.rules()), ); assert_messages!(snapshot, diagnostics); } diff --git a/crates/ruff/src/rules/pandas_vet/mod.rs b/crates/ruff/src/rules/pandas_vet/mod.rs index 2032749fe28ab..295a83cd24086 100644 --- a/crates/ruff/src/rules/pandas_vet/mod.rs +++ b/crates/ruff/src/rules/pandas_vet/mod.rs @@ -353,8 +353,10 @@ mod tests { "PD901_fail_df_var" )] fn contents(contents: &str, snapshot: &str) { - let diagnostics = - test_snippet(contents, &settings::Settings::for_rules(&Linter::PandasVet)); + let diagnostics = test_snippet( + contents, + &settings::Settings::for_rules(Linter::PandasVet.rules()), + ); assert_messages!(snapshot, diagnostics); } diff --git a/crates/ruff/src/rules/pyflakes/mod.rs b/crates/ruff/src/rules/pyflakes/mod.rs index 0bdbeaee1d119..e796ac45639fd 100644 --- a/crates/ruff/src/rules/pyflakes/mod.rs +++ b/crates/ruff/src/rules/pyflakes/mod.rs @@ -490,7 +490,7 @@ mod tests { "load_after_unbind_from_class_scope" )] fn contents(contents: &str, snapshot: &str) { - let diagnostics = test_snippet(contents, &Settings::for_rules(&Linter::Pyflakes)); + let diagnostics = test_snippet(contents, &Settings::for_rules(Linter::Pyflakes.rules())); assert_messages!(snapshot, diagnostics); } @@ -498,7 +498,7 @@ mod tests { /// Note that all tests marked with `#[ignore]` should be considered TODOs. fn flakes(contents: &str, expected: &[Rule]) { let contents = dedent(contents); - let settings = Settings::for_rules(&Linter::Pyflakes); + let settings = Settings::for_rules(Linter::Pyflakes.rules()); let tokens: Vec = ruff_rustpython::tokenize(&contents); let locator = Locator::new(&contents); let stylist = Stylist::from_tokens(&tokens, &locator); diff --git a/crates/ruff_dev/src/generate_rules_table.rs b/crates/ruff_dev/src/generate_rules_table.rs index 4d29bfea2bf3a..6f1b18cfea6b2 100644 --- a/crates/ruff_dev/src/generate_rules_table.rs +++ b/crates/ruff_dev/src/generate_rules_table.rs @@ -102,10 +102,10 @@ pub(crate) fn generate() -> String { )); table_out.push('\n'); table_out.push('\n'); - generate_table(&mut table_out, prefix, &linter); + generate_table(&mut table_out, prefix.clone().rules(), &linter); } } else { - generate_table(&mut table_out, &linter, &linter); + generate_table(&mut table_out, linter.rules(), &linter); } } diff --git a/crates/ruff_macros/src/map_codes.rs b/crates/ruff_macros/src/map_codes.rs index d37113e0aadf8..eeee0d7ac96cb 100644 --- a/crates/ruff_macros/src/map_codes.rs +++ b/crates/ruff_macros/src/map_codes.rs @@ -155,30 +155,13 @@ pub(crate) fn map_codes(func: &ItemFn) -> syn::Result { } output.extend(quote! { - impl IntoIterator for &#linter { - type Item = Rule; - type IntoIter = ::std::vec::IntoIter; - - fn into_iter(self) -> Self::IntoIter { + impl #linter { + pub fn rules(self) -> ::std::vec::IntoIter { match self { #prefix_into_iter_match_arms } } } }); } - - output.extend(quote! { - impl IntoIterator for &RuleCodePrefix { - type Item = Rule; - type IntoIter = ::std::vec::IntoIter; - - fn into_iter(self) -> Self::IntoIter { - match self { - #(RuleCodePrefix::#linter_idents(prefix) => prefix.into_iter(),)* - } - } - } - }); - output.extend(quote! { impl RuleCodePrefix { pub fn parse(linter: &Linter, code: &str) -> Result { @@ -188,6 +171,12 @@ pub(crate) fn map_codes(func: &ItemFn) -> syn::Result { #(Linter::#linter_idents => RuleCodePrefix::#linter_idents(#linter_idents::from_str(code).map_err(|_| crate::registry::FromCodeError::Unknown)?),)* }) } + + pub fn rules(self) -> ::std::vec::IntoIter { + match self { + #(RuleCodePrefix::#linter_idents(prefix) => prefix.clone().rules(),)* + } + } } }); @@ -344,32 +333,39 @@ fn generate_iter_impl( linter_to_rules: &BTreeMap>, linter_idents: &[&Ident], ) -> TokenStream { - let mut linter_into_iter_match_arms = quote!(); + let mut linter_rules_match_arms = quote!(); + let mut linter_all_rules_match_arms = quote!(); for (linter, map) in linter_to_rules { - let rule_paths = map - .values() - .filter(|rule| { - // Nursery rules have to be explicitly selected, so we ignore them when looking at - // linter-level selectors (e.g., `--select SIM`). - !is_nursery(&rule.group) - }) - .map(|Rule { attrs, path, .. }| { + let rule_paths = map.values().filter(|rule| !is_nursery(&rule.group)).map( + |Rule { attrs, path, .. }| { let rule_name = path.segments.last().unwrap(); quote!(#(#attrs)* Rule::#rule_name) - }); - linter_into_iter_match_arms.extend(quote! { + }, + ); + linter_rules_match_arms.extend(quote! { + Linter::#linter => vec![#(#rule_paths,)*].into_iter(), + }); + let rule_paths = map.values().map(|Rule { attrs, path, .. }| { + let rule_name = path.segments.last().unwrap(); + quote!(#(#attrs)* Rule::#rule_name) + }); + linter_all_rules_match_arms.extend(quote! { Linter::#linter => vec![#(#rule_paths,)*].into_iter(), }); } quote! { - impl IntoIterator for &Linter { - type Item = Rule; - type IntoIter = ::std::vec::IntoIter; - - fn into_iter(self) -> Self::IntoIter { + impl Linter { + /// Rules not in the nursery. + pub fn rules(self: &Linter) -> ::std::vec::IntoIter { + match self { + #linter_rules_match_arms + } + } + /// All rules, including those in the nursery. + pub fn all_rules(self: &Linter) -> ::std::vec::IntoIter { match self { - #linter_into_iter_match_arms + #linter_all_rules_match_arms } } } From 787e2fd49d49b38f4ceb8f5f882caa20050c4f5f Mon Sep 17 00:00:00 2001 From: konsti Date: Tue, 4 Jul 2023 09:07:20 +0200 Subject: [PATCH 03/15] Format import statements (#5493) ## Summary Format import statements in all their variants. Specifically, this implemented formatting `StmtImport`, `StmtImportFrom` and `Alias`. ## Test Plan I added some custom snapshots, even though this has been covered well by black's tests. --- .../test/fixtures/ruff/statement/import.py | 3 + .../fixtures/ruff/statement/import_from.py | 16 + .../ruff_python_formatter/src/other/alias.rs | 16 +- .../src/statement/stmt_import.rs | 11 +- .../src/statement/stmt_import_from.rs | 42 ++- ...atibility@miscellaneous__force_pyi.py.snap | 23 +- ...ty@py_310__pattern_matching_extras.py.snap | 27 +- ...tibility@simple_cases__collections.py.snap | 47 +-- ...mpatibility@simple_cases__comments.py.snap | 347 ------------------ ...patibility@simple_cases__comments2.py.snap | 35 +- ...patibility@simple_cases__comments4.py.snap | 22 +- ...patibility@simple_cases__comments5.py.snap | 255 ------------- ...patibility@simple_cases__comments6.py.snap | 8 +- ...cases__comments_non_breaking_space.py.snap | 20 +- ...mpatibility@simple_cases__fmtonoff.py.snap | 29 +- ...patibility@simple_cases__fmtonoff2.py.snap | 9 +- ...lity@simple_cases__fmtpass_imports.py.snap | 114 ------ ...mpatibility@simple_cases__function.py.snap | 22 +- ...patibility@simple_cases__function2.py.snap | 17 +- ...ility@simple_cases__import_spacing.py.snap | 146 +++----- ...@simple_cases__remove_await_parens.py.snap | 8 +- ...move_newline_after_code_block_open.py.snap | 8 +- .../format@expression__attribute.py.snap | 2 +- .../snapshots/format@expression__call.py.snap | 2 +- .../format@statement__import.py.snap | 28 ++ .../format@statement__import_from.py.snap | 46 +++ 26 files changed, 308 insertions(+), 995 deletions(-) create mode 100644 crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/import.py create mode 100644 crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/import_from.py delete mode 100644 crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments.py.snap delete mode 100644 crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments5.py.snap delete mode 100644 crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtpass_imports.py.snap create mode 100644 crates/ruff_python_formatter/tests/snapshots/format@statement__import.py.snap create mode 100644 crates/ruff_python_formatter/tests/snapshots/format@statement__import_from.py.snap diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/import.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/import.py new file mode 100644 index 0000000000000..1677400431dd0 --- /dev/null +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/import.py @@ -0,0 +1,3 @@ +from a import aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa +from a import aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa +from a import aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa as dfgsdfgsd, aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa as sdkjflsdjlahlfd diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/import_from.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/import_from.py new file mode 100644 index 0000000000000..335e91036abf9 --- /dev/null +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/import_from.py @@ -0,0 +1,16 @@ +from a import aksjdhflsakhdflkjsadlfajkslhf +from a import ( + aksjdhflsakhdflkjsadlfajkslhf, +) +from a import ( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, +) +from a import ( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, +) +from a import ( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa as dfgsdfgsd, + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa as sdkjflsdjlahlfd, +) +from aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa import * diff --git a/crates/ruff_python_formatter/src/other/alias.rs b/crates/ruff_python_formatter/src/other/alias.rs index 8a1501e09c66b..f59dd012bdebf 100644 --- a/crates/ruff_python_formatter/src/other/alias.rs +++ b/crates/ruff_python_formatter/src/other/alias.rs @@ -1,5 +1,6 @@ -use crate::{not_yet_implemented, FormatNodeRule, PyFormatter}; -use ruff_formatter::{write, Buffer, FormatResult}; +use crate::{AsFormat, FormatNodeRule, PyFormatter}; +use ruff_formatter::prelude::{space, text}; +use ruff_formatter::{write, Buffer, Format, FormatResult}; use rustpython_parser::ast::Alias; #[derive(Default)] @@ -7,6 +8,15 @@ pub struct FormatAlias; impl FormatNodeRule for FormatAlias { fn fmt_fields(&self, item: &Alias, f: &mut PyFormatter) -> FormatResult<()> { - write!(f, [not_yet_implemented(item)]) + let Alias { + range: _, + name, + asname, + } = item; + name.format().fmt(f)?; + if let Some(asname) = asname { + write!(f, [space(), text("as"), space(), asname.format()])?; + } + Ok(()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_import.rs b/crates/ruff_python_formatter/src/statement/stmt_import.rs index 2585dfade7c35..10aec39721759 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_import.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_import.rs @@ -1,4 +1,5 @@ -use crate::{not_yet_implemented, FormatNodeRule, PyFormatter}; +use crate::{FormatNodeRule, FormattedIterExt, PyFormatter}; +use ruff_formatter::prelude::{format_args, format_with, space, text}; use ruff_formatter::{write, Buffer, FormatResult}; use rustpython_parser::ast::StmtImport; @@ -7,6 +8,12 @@ pub struct FormatStmtImport; impl FormatNodeRule for FormatStmtImport { fn fmt_fields(&self, item: &StmtImport, f: &mut PyFormatter) -> FormatResult<()> { - write!(f, [not_yet_implemented(item)]) + let StmtImport { names, range: _ } = item; + let names = format_with(|f| { + f.join_with(&format_args![text(","), space()]) + .entries(names.iter().formatted()) + .finish() + }); + write!(f, [text("import"), space(), names]) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_import_from.rs b/crates/ruff_python_formatter/src/statement/stmt_import_from.rs index bdae4d56bab37..ef8cc13584492 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_import_from.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_import_from.rs @@ -1,5 +1,7 @@ -use crate::{not_yet_implemented, FormatNodeRule, PyFormatter}; -use ruff_formatter::{write, Buffer, FormatResult}; +use crate::builders::{optional_parentheses, PyFormatterExtensions}; +use crate::{AsFormat, FormatNodeRule, PyFormatter}; +use ruff_formatter::prelude::{dynamic_text, format_with, space, text}; +use ruff_formatter::{write, Buffer, Format, FormatResult}; use rustpython_parser::ast::StmtImportFrom; #[derive(Default)] @@ -7,6 +9,40 @@ pub struct FormatStmtImportFrom; impl FormatNodeRule for FormatStmtImportFrom { fn fmt_fields(&self, item: &StmtImportFrom, f: &mut PyFormatter) -> FormatResult<()> { - write!(f, [not_yet_implemented(item)]) + let StmtImportFrom { + module, + names, + range: _, + level, + } = item; + + let level_str = level + .map(|level| ".".repeat(level.to_usize())) + .unwrap_or(String::default()); + + write!( + f, + [ + text("from"), + space(), + dynamic_text(&level_str, None), + module.as_ref().map(AsFormat::format), + space(), + text("import"), + space(), + ] + )?; + if let [name] = names.as_slice() { + // star can't be surrounded by parentheses + if name.name.as_str() == "*" { + return text("*").fmt(f); + } + } + let names = format_with(|f| { + f.join_comma_separated() + .entries(names.iter().map(|name| (name, name.format()))) + .finish() + }); + optional_parentheses(&names).fmt(f) } } diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@miscellaneous__force_pyi.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@miscellaneous__force_pyi.py.snap index 52fbac942f99d..f8f00344e54de 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@miscellaneous__force_pyi.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@miscellaneous__force_pyi.py.snap @@ -43,21 +43,20 @@ def eggs() -> Union[str, int]: ... --- Black +++ Ruff @@ -1,32 +1,58 @@ --from typing import Union -+NOT_YET_IMPLEMENTED_StmtImportFrom -+ + from typing import Union ++ @bird -def zoo(): ... +def zoo(): + ... -+ -+ -+class A: -+ ... -class A: ... ++class A: ++ ... ++ ++ @bar class B: - def BMethod(self) -> None: ... @@ -94,14 +93,14 @@ def eggs() -> Union[str, int]: ... + +class F(A, C): + ... ++ ++ ++def spam() -> None: ++ ... -class F(A, C): ... -def spam() -> None: ... -+def spam() -> None: -+ ... -+ -+ @overload -def spam(arg: str) -> str: ... +def spam(arg: str) -> str: @@ -120,7 +119,7 @@ def eggs() -> Union[str, int]: ... ## Ruff Output ```py -NOT_YET_IMPLEMENTED_StmtImportFrom +from typing import Union @bird diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_310__pattern_matching_extras.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_310__pattern_matching_extras.py.snap index 9afb2c247d9b1..8752abc340822 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_310__pattern_matching_extras.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_310__pattern_matching_extras.py.snap @@ -132,8 +132,7 @@ match bar1: --- Black +++ Ruff @@ -1,119 +1,43 @@ --import match -+NOT_YET_IMPLEMENTED_StmtImport + import match -match something: - case [a as b]: @@ -208,10 +207,11 @@ match bar1: - ), - ): - pass -- ++NOT_YET_IMPLEMENTED_StmtMatch + - case [a as match]: - pass -- + - case case: - pass +NOT_YET_IMPLEMENTED_StmtMatch @@ -220,8 +220,9 @@ match bar1: -match match: - case case: - pass -- -- ++NOT_YET_IMPLEMENTED_StmtMatch + + -match a, *b(), c: - case d, *f, g: - pass @@ -236,30 +237,28 @@ match bar1: - pass - case {"maybe": something(complicated as this) as that}: - pass +- +NOT_YET_IMPLEMENTED_StmtMatch - -match something: - case 1 as a: - pass -+NOT_YET_IMPLEMENTED_StmtMatch - case 2 as b, 3 as c: - pass ++NOT_YET_IMPLEMENTED_StmtMatch - case 4 as d, (5 as e), (6 | 7 as g), *h: - pass -+NOT_YET_IMPLEMENTED_StmtMatch - +- -match bar1: - case Foo(aa=Callable() as aa, bb=int()): - print(bar1.aa, bar1.bb) - case _: - print("no match", "\n") -+NOT_YET_IMPLEMENTED_StmtMatch - - +- +- -match bar1: - case Foo( - normal=x, perhaps=[list, {"x": d, "y": 1.0}] as y, otherwise=something, q=t as u @@ -271,7 +270,7 @@ match bar1: ## Ruff Output ```py -NOT_YET_IMPLEMENTED_StmtImport +import match NOT_YET_IMPLEMENTED_StmtMatch diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__collections.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__collections.py.snap index 51c59c4e9d7b1..cf6666738208c 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__collections.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__collections.py.snap @@ -83,31 +83,9 @@ if True: ```diff --- Black +++ Ruff -@@ -1,40 +1,22 @@ --import core, time, a -+NOT_YET_IMPLEMENTED_StmtImport - --from . import A, B, C -+NOT_YET_IMPLEMENTED_StmtImportFrom - - # keeps existing trailing comma --from foo import ( -- bar, --) -+NOT_YET_IMPLEMENTED_StmtImportFrom - - # also keeps existing structure --from foo import ( -- baz, -- qux, --) -+NOT_YET_IMPLEMENTED_StmtImportFrom - - # `as` works as well --from foo import ( -- xyzzy as magic, --) -+NOT_YET_IMPLEMENTED_StmtImportFrom +@@ -18,23 +18,12 @@ + xyzzy as magic, + ) -a = { - 1, @@ -132,7 +110,7 @@ if True: nested_no_trailing_comma = {(1, 2, 3), (4, 5, 6)} nested_long_lines = [ "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", -@@ -52,10 +34,7 @@ +@@ -52,10 +41,7 @@ y = { "oneple": (1,), } @@ -149,18 +127,25 @@ if True: ## Ruff Output ```py -NOT_YET_IMPLEMENTED_StmtImport +import core, time, a -NOT_YET_IMPLEMENTED_StmtImportFrom +from . import A, B, C # keeps existing trailing comma -NOT_YET_IMPLEMENTED_StmtImportFrom +from foo import ( + bar, +) # also keeps existing structure -NOT_YET_IMPLEMENTED_StmtImportFrom +from foo import ( + baz, + qux, +) # `as` works as well -NOT_YET_IMPLEMENTED_StmtImportFrom +from foo import ( + xyzzy as magic, +) a = {1, 2, 3} b = {1, 2, 3} diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments.py.snap deleted file mode 100644 index d78df8d356e01..0000000000000 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments.py.snap +++ /dev/null @@ -1,347 +0,0 @@ ---- -source: crates/ruff_python_formatter/tests/fixtures.rs -input_file: crates/ruff_python_formatter/resources/test/fixtures/black/simple_cases/comments.py ---- -## Input - -```py -#!/usr/bin/env python3 -# fmt: on -# Some license here. -# -# Has many lines. Many, many lines. -# Many, many, many lines. -"""Module docstring. - -Possibly also many, many lines. -""" - -import os.path -import sys - -import a -from b.c import X # some noqa comment - -try: - import fast -except ImportError: - import slow as fast - - -# Some comment before a function. -y = 1 -( - # some strings - y # type: ignore -) - - -def function(default=None): - """Docstring comes first. - - Possibly many lines. - """ - # FIXME: Some comment about why this function is crap but still in production. - import inner_imports - - if inner_imports.are_evil(): - # Explains why we have this if. - # In great detail indeed. - x = X() - return x.method1() # type: ignore - - # This return is also commented for some reason. - return default - - -# Explains why we use global state. -GLOBAL_STATE = {"a": a(1), "b": a(2), "c": a(3)} - - -# Another comment! -# This time two lines. - - -class Foo: - """Docstring for class Foo. Example from Sphinx docs.""" - - #: Doc comment for class attribute Foo.bar. - #: It can have multiple lines. - bar = 1 - - flox = 1.5 #: Doc comment for Foo.flox. One line only. - - baz = 2 - """Docstring for class attribute Foo.baz.""" - - def __init__(self): - #: Doc comment for instance attribute qux. - self.qux = 3 - - self.spam = 4 - """Docstring for instance attribute spam.""" - - -#'

This is pweave!

- - -@fast(really=True) -async def wat(): - # This comment, for some reason \ - # contains a trailing backslash. - async with X.open_async() as x: # Some more comments - result = await x.method1() - # Comment after ending a block. - if result: - print("A OK", file=sys.stdout) - # Comment between things. - print() - - -# Some closing comments. -# Maybe Vim or Emacs directives for formatting. -# Who knows. -``` - -## Black Differences - -```diff ---- Black -+++ Ruff -@@ -9,16 +9,16 @@ - Possibly also many, many lines. - """ - --import os.path --import sys -+NOT_YET_IMPLEMENTED_StmtImport -+NOT_YET_IMPLEMENTED_StmtImport - --import a --from b.c import X # some noqa comment -+NOT_YET_IMPLEMENTED_StmtImport -+NOT_YET_IMPLEMENTED_StmtImportFrom # some noqa comment - - try: -- import fast -+ NOT_YET_IMPLEMENTED_StmtImport - except ImportError: -- import slow as fast -+ NOT_YET_IMPLEMENTED_StmtImport - - - # Some comment before a function. -@@ -35,7 +35,7 @@ - Possibly many lines. - """ - # FIXME: Some comment about why this function is crap but still in production. -- import inner_imports -+ NOT_YET_IMPLEMENTED_StmtImport - - if inner_imports.are_evil(): - # Explains why we have this if. -``` - -## Ruff Output - -```py -#!/usr/bin/env python3 -# fmt: on -# Some license here. -# -# Has many lines. Many, many lines. -# Many, many, many lines. -"""Module docstring. - -Possibly also many, many lines. -""" - -NOT_YET_IMPLEMENTED_StmtImport -NOT_YET_IMPLEMENTED_StmtImport - -NOT_YET_IMPLEMENTED_StmtImport -NOT_YET_IMPLEMENTED_StmtImportFrom # some noqa comment - -try: - NOT_YET_IMPLEMENTED_StmtImport -except ImportError: - NOT_YET_IMPLEMENTED_StmtImport - - -# Some comment before a function. -y = 1 -( - # some strings - y # type: ignore -) - - -def function(default=None): - """Docstring comes first. - - Possibly many lines. - """ - # FIXME: Some comment about why this function is crap but still in production. - NOT_YET_IMPLEMENTED_StmtImport - - if inner_imports.are_evil(): - # Explains why we have this if. - # In great detail indeed. - x = X() - return x.method1() # type: ignore - - # This return is also commented for some reason. - return default - - -# Explains why we use global state. -GLOBAL_STATE = {"a": a(1), "b": a(2), "c": a(3)} - - -# Another comment! -# This time two lines. - - -class Foo: - """Docstring for class Foo. Example from Sphinx docs.""" - - #: Doc comment for class attribute Foo.bar. - #: It can have multiple lines. - bar = 1 - - flox = 1.5 #: Doc comment for Foo.flox. One line only. - - baz = 2 - """Docstring for class attribute Foo.baz.""" - - def __init__(self): - #: Doc comment for instance attribute qux. - self.qux = 3 - - self.spam = 4 - """Docstring for instance attribute spam.""" - - -#'

This is pweave!

- - -@fast(really=True) -async def wat(): - # This comment, for some reason \ - # contains a trailing backslash. - async with X.open_async() as x: # Some more comments - result = await x.method1() - # Comment after ending a block. - if result: - print("A OK", file=sys.stdout) - # Comment between things. - print() - - -# Some closing comments. -# Maybe Vim or Emacs directives for formatting. -# Who knows. -``` - -## Black Output - -```py -#!/usr/bin/env python3 -# fmt: on -# Some license here. -# -# Has many lines. Many, many lines. -# Many, many, many lines. -"""Module docstring. - -Possibly also many, many lines. -""" - -import os.path -import sys - -import a -from b.c import X # some noqa comment - -try: - import fast -except ImportError: - import slow as fast - - -# Some comment before a function. -y = 1 -( - # some strings - y # type: ignore -) - - -def function(default=None): - """Docstring comes first. - - Possibly many lines. - """ - # FIXME: Some comment about why this function is crap but still in production. - import inner_imports - - if inner_imports.are_evil(): - # Explains why we have this if. - # In great detail indeed. - x = X() - return x.method1() # type: ignore - - # This return is also commented for some reason. - return default - - -# Explains why we use global state. -GLOBAL_STATE = {"a": a(1), "b": a(2), "c": a(3)} - - -# Another comment! -# This time two lines. - - -class Foo: - """Docstring for class Foo. Example from Sphinx docs.""" - - #: Doc comment for class attribute Foo.bar. - #: It can have multiple lines. - bar = 1 - - flox = 1.5 #: Doc comment for Foo.flox. One line only. - - baz = 2 - """Docstring for class attribute Foo.baz.""" - - def __init__(self): - #: Doc comment for instance attribute qux. - self.qux = 3 - - self.spam = 4 - """Docstring for instance attribute spam.""" - - -#'

This is pweave!

- - -@fast(really=True) -async def wat(): - # This comment, for some reason \ - # contains a trailing backslash. - async with X.open_async() as x: # Some more comments - result = await x.method1() - # Comment after ending a block. - if result: - print("A OK", file=sys.stdout) - # Comment between things. - print() - - -# Some closing comments. -# Maybe Vim or Emacs directives for formatting. -# Who knows. -``` - - diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments2.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments2.py.snap index 23020bfa282f5..e7ae03c930383 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments2.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments2.py.snap @@ -177,19 +177,18 @@ instruction()#comment with bad spacing ```diff --- Black +++ Ruff -@@ -1,9 +1,5 @@ --from com.my_lovely_company.my_lovely_team.my_lovely_project.my_lovely_component import ( +@@ -1,8 +1,8 @@ + from com.my_lovely_company.my_lovely_team.my_lovely_project.my_lovely_component import ( - MyLovelyCompanyTeamProjectComponent, # NOT DRY --) --from com.my_lovely_company.my_lovely_team.my_lovely_project.my_lovely_component import ( ++ MyLovelyCompanyTeamProjectComponent # NOT DRY + ) + from com.my_lovely_company.my_lovely_team.my_lovely_project.my_lovely_component import ( - MyLovelyCompanyTeamProjectComponent as component, # DRY --) -+NOT_YET_IMPLEMENTED_StmtImportFrom -+NOT_YET_IMPLEMENTED_StmtImportFrom ++ MyLovelyCompanyTeamProjectComponent as component # DRY + ) # Please keep __all__ alphabetized within each category. - -@@ -45,7 +41,7 @@ +@@ -45,7 +45,7 @@ # user-defined types and objects Cheese, Cheese("Wensleydale"), @@ -198,7 +197,7 @@ instruction()#comment with bad spacing ] if "PYTHON" in os.environ: -@@ -60,8 +56,12 @@ +@@ -60,8 +60,12 @@ # Comment before function. def inline_comments_in_brackets_ruin_everything(): if typedargslist: @@ -212,7 +211,7 @@ instruction()#comment with bad spacing children[0], body, children[-1], # type: ignore -@@ -72,7 +72,11 @@ +@@ -72,7 +76,11 @@ body, parameters.children[-1], # )2 ] @@ -225,7 +224,7 @@ instruction()#comment with bad spacing if ( self._proc is not None # has the child process finished? -@@ -114,25 +118,9 @@ +@@ -114,25 +122,9 @@ # yup arg3=True, ) @@ -254,7 +253,7 @@ instruction()#comment with bad spacing while True: if False: continue -@@ -143,7 +131,10 @@ +@@ -143,7 +135,10 @@ # let's return return Node( syms.simple_stmt, @@ -266,7 +265,7 @@ instruction()#comment with bad spacing ) -@@ -158,7 +149,11 @@ +@@ -158,7 +153,11 @@ class Test: def _init_host(self, parsed) -> None: @@ -284,8 +283,12 @@ instruction()#comment with bad spacing ## Ruff Output ```py -NOT_YET_IMPLEMENTED_StmtImportFrom -NOT_YET_IMPLEMENTED_StmtImportFrom +from com.my_lovely_company.my_lovely_team.my_lovely_project.my_lovely_component import ( + MyLovelyCompanyTeamProjectComponent # NOT DRY +) +from com.my_lovely_company.my_lovely_team.my_lovely_project.my_lovely_component import ( + MyLovelyCompanyTeamProjectComponent as component # DRY +) # Please keep __all__ alphabetized within each category. diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments4.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments4.py.snap index 2358992e6cfe4..7a1948330ffe6 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments4.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments4.py.snap @@ -106,19 +106,7 @@ def foo3(list_a, list_b): ```diff --- Black +++ Ruff -@@ -1,9 +1,5 @@ --from com.my_lovely_company.my_lovely_team.my_lovely_project.my_lovely_component import ( -- MyLovelyCompanyTeamProjectComponent, # NOT DRY --) --from com.my_lovely_company.my_lovely_team.my_lovely_project.my_lovely_component import ( -- MyLovelyCompanyTeamProjectComponent as component, # DRY --) -+NOT_YET_IMPLEMENTED_StmtImportFrom -+NOT_YET_IMPLEMENTED_StmtImportFrom - - - class C: -@@ -58,37 +54,28 @@ +@@ -58,37 +58,28 @@ def foo(list_a, list_b): results = ( @@ -172,8 +160,12 @@ def foo3(list_a, list_b): ## Ruff Output ```py -NOT_YET_IMPLEMENTED_StmtImportFrom -NOT_YET_IMPLEMENTED_StmtImportFrom +from com.my_lovely_company.my_lovely_team.my_lovely_project.my_lovely_component import ( + MyLovelyCompanyTeamProjectComponent, # NOT DRY +) +from com.my_lovely_company.my_lovely_team.my_lovely_project.my_lovely_component import ( + MyLovelyCompanyTeamProjectComponent as component, # DRY +) class C: diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments5.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments5.py.snap deleted file mode 100644 index 28f4426951396..0000000000000 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments5.py.snap +++ /dev/null @@ -1,255 +0,0 @@ ---- -source: crates/ruff_python_formatter/tests/fixtures.rs -input_file: crates/ruff_python_formatter/resources/test/fixtures/black/simple_cases/comments5.py ---- -## Input - -```py -while True: - if something.changed: - do.stuff() # trailing comment - # Comment belongs to the `if` block. - # This one belongs to the `while` block. - - # Should this one, too? I guess so. - -# This one is properly standalone now. - -for i in range(100): - # first we do this - if i % 33 == 0: - break - - # then we do this - print(i) - # and finally we loop around - -with open(some_temp_file) as f: - data = f.read() - -try: - with open(some_other_file) as w: - w.write(data) - -except OSError: - print("problems") - -import sys - - -# leading function comment -def wat(): - ... - # trailing function comment - - -# SECTION COMMENT - - -# leading 1 -@deco1 -# leading 2 -@deco2(with_args=True) -# leading 3 -@deco3 -def decorated1(): - ... - - -# leading 1 -@deco1 -# leading 2 -@deco2(with_args=True) -# leading function comment -def decorated1(): - ... - - -# Note: this is fixed in -# Preview.empty_lines_before_class_or_def_with_leading_comments. -# In the current style, the user will have to split those lines by hand. -some_instruction - - -# This comment should be split from `some_instruction` by two lines but isn't. -def g(): - ... - - -if __name__ == "__main__": - main() -``` - -## Black Differences - -```diff ---- Black -+++ Ruff -@@ -27,7 +27,7 @@ - except OSError: - print("problems") - --import sys -+NOT_YET_IMPLEMENTED_StmtImport - - - # leading function comment -``` - -## Ruff Output - -```py -while True: - if something.changed: - do.stuff() # trailing comment - # Comment belongs to the `if` block. - # This one belongs to the `while` block. - - # Should this one, too? I guess so. - -# This one is properly standalone now. - -for i in range(100): - # first we do this - if i % 33 == 0: - break - - # then we do this - print(i) - # and finally we loop around - -with open(some_temp_file) as f: - data = f.read() - -try: - with open(some_other_file) as w: - w.write(data) - -except OSError: - print("problems") - -NOT_YET_IMPLEMENTED_StmtImport - - -# leading function comment -def wat(): - ... - # trailing function comment - - -# SECTION COMMENT - - -# leading 1 -@deco1 -# leading 2 -@deco2(with_args=True) -# leading 3 -@deco3 -def decorated1(): - ... - - -# leading 1 -@deco1 -# leading 2 -@deco2(with_args=True) -# leading function comment -def decorated1(): - ... - - -# Note: this is fixed in -# Preview.empty_lines_before_class_or_def_with_leading_comments. -# In the current style, the user will have to split those lines by hand. -some_instruction - - -# This comment should be split from `some_instruction` by two lines but isn't. -def g(): - ... - - -if __name__ == "__main__": - main() -``` - -## Black Output - -```py -while True: - if something.changed: - do.stuff() # trailing comment - # Comment belongs to the `if` block. - # This one belongs to the `while` block. - - # Should this one, too? I guess so. - -# This one is properly standalone now. - -for i in range(100): - # first we do this - if i % 33 == 0: - break - - # then we do this - print(i) - # and finally we loop around - -with open(some_temp_file) as f: - data = f.read() - -try: - with open(some_other_file) as w: - w.write(data) - -except OSError: - print("problems") - -import sys - - -# leading function comment -def wat(): - ... - # trailing function comment - - -# SECTION COMMENT - - -# leading 1 -@deco1 -# leading 2 -@deco2(with_args=True) -# leading 3 -@deco3 -def decorated1(): - ... - - -# leading 1 -@deco1 -# leading 2 -@deco2(with_args=True) -# leading function comment -def decorated1(): - ... - - -# Note: this is fixed in -# Preview.empty_lines_before_class_or_def_with_leading_comments. -# In the current style, the user will have to split those lines by hand. -some_instruction - - -# This comment should be split from `some_instruction` by two lines but isn't. -def g(): - ... - - -if __name__ == "__main__": - main() -``` - - diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments6.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments6.py.snap index 511d9fe1b1673..a27f99c5bf93e 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments6.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments6.py.snap @@ -130,12 +130,6 @@ aaaaaaaaaaaaa, bbbbbbbbb = map(list, map(itertools.chain.from_iterable, zip(*ite ```diff --- Black +++ Ruff -@@ -1,4 +1,4 @@ --from typing import Any, Tuple -+NOT_YET_IMPLEMENTED_StmtImportFrom - - - def f( @@ -49,9 +49,7 @@ element = 0 # type: int another_element = 1 # type: float @@ -192,7 +186,7 @@ aaaaaaaaaaaaa, bbbbbbbbb = map(list, map(itertools.chain.from_iterable, zip(*ite ## Ruff Output ```py -NOT_YET_IMPLEMENTED_StmtImportFrom +from typing import Any, Tuple def f( diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments_non_breaking_space.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments_non_breaking_space.py.snap index 12fd5c1269dca..e8c73055d0c21 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments_non_breaking_space.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments_non_breaking_space.py.snap @@ -31,18 +31,7 @@ def function(a:int=42): ```diff --- Black +++ Ruff -@@ -1,9 +1,4 @@ --from .config import ( -- ConfigTypeAttributes, -- Int, -- Path, # String, -- # DEFAULT_TYPE_ATTRIBUTES, --) -+NOT_YET_IMPLEMENTED_StmtImportFrom - - result = 1 # A simple comment - result = (1,) # Another one -@@ -14,9 +9,9 @@ +@@ -14,9 +14,9 @@ def function(a: int = 42): @@ -60,7 +49,12 @@ def function(a:int=42): ## Ruff Output ```py -NOT_YET_IMPLEMENTED_StmtImportFrom +from .config import ( + ConfigTypeAttributes, + Int, + Path, # String, + # DEFAULT_TYPE_ATTRIBUTES, +) result = 1 # A simple comment result = (1,) # Another one diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff.py.snap index 32082f2da9457..013a97677be58 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff.py.snap @@ -198,22 +198,13 @@ d={'a':1, ```diff --- Black +++ Ruff -@@ -1,15 +1,14 @@ - #!/usr/bin/env python3 --import asyncio --import sys -+NOT_YET_IMPLEMENTED_StmtImport -+NOT_YET_IMPLEMENTED_StmtImport +@@ -6,10 +6,9 @@ --from third_party import X, Y, Z -+NOT_YET_IMPLEMENTED_StmtImportFrom - --from library import some_connection, some_decorator -+NOT_YET_IMPLEMENTED_StmtImportFrom + from library import some_connection, some_decorator # fmt: off -from third_party import (X, - Y, Z) -+NOT_YET_IMPLEMENTED_StmtImportFrom ++from third_party import X, Y, Z # fmt: on -f"trigger 3.6 mode" +NOT_YET_IMPLEMENTED_ExprJoinedStr @@ -336,7 +327,7 @@ d={'a':1, # fmt: off - from hello import a, b - 'unformatted' -+ NOT_YET_IMPLEMENTED_StmtImportFrom ++ from hello import a, b + "unformatted" # fmt: on @@ -433,14 +424,14 @@ d={'a':1, ```py #!/usr/bin/env python3 -NOT_YET_IMPLEMENTED_StmtImport -NOT_YET_IMPLEMENTED_StmtImport +import asyncio +import sys -NOT_YET_IMPLEMENTED_StmtImportFrom +from third_party import X, Y, Z -NOT_YET_IMPLEMENTED_StmtImportFrom +from library import some_connection, some_decorator # fmt: off -NOT_YET_IMPLEMENTED_StmtImportFrom +from third_party import X, Y, Z # fmt: on NOT_YET_IMPLEMENTED_ExprJoinedStr # Comment 1 @@ -539,7 +530,7 @@ def subscriptlist(): def import_as_names(): # fmt: off - NOT_YET_IMPLEMENTED_StmtImportFrom + from hello import a, b "unformatted" # fmt: on diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff2.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff2.py.snap index 56b345fae1b2d..abdfcb6d563f9 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff2.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtonoff2.py.snap @@ -52,12 +52,7 @@ def test_calculate_fades(): ```diff --- Black +++ Ruff -@@ -1,40 +1,44 @@ --import pytest -+NOT_YET_IMPLEMENTED_StmtImport - - TmSt = 1 - TmEx = 2 +@@ -5,36 +5,40 @@ # fmt: off @@ -113,7 +108,7 @@ def test_calculate_fades(): ## Ruff Output ```py -NOT_YET_IMPLEMENTED_StmtImport +import pytest TmSt = 1 TmEx = 2 diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtpass_imports.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtpass_imports.py.snap deleted file mode 100644 index 5a0bfd673fc08..0000000000000 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtpass_imports.py.snap +++ /dev/null @@ -1,114 +0,0 @@ ---- -source: crates/ruff_python_formatter/tests/fixtures.rs -input_file: crates/ruff_python_formatter/resources/test/fixtures/black/simple_cases/fmtpass_imports.py ---- -## Input - -```py -# Regression test for https://github.com/psf/black/issues/3438 - -import ast -import collections # fmt: skip -import dataclasses -# fmt: off -import os -# fmt: on -import pathlib - -import re # fmt: skip -import secrets - -# fmt: off -import sys -# fmt: on - -import tempfile -import zoneinfo -``` - -## Black Differences - -```diff ---- Black -+++ Ruff -@@ -1,19 +1,19 @@ - # Regression test for https://github.com/psf/black/issues/3438 - --import ast --import collections # fmt: skip --import dataclasses -+NOT_YET_IMPLEMENTED_StmtImport -+NOT_YET_IMPLEMENTED_StmtImport # fmt: skip -+NOT_YET_IMPLEMENTED_StmtImport - # fmt: off --import os -+NOT_YET_IMPLEMENTED_StmtImport - # fmt: on --import pathlib -+NOT_YET_IMPLEMENTED_StmtImport - --import re # fmt: skip --import secrets -+NOT_YET_IMPLEMENTED_StmtImport # fmt: skip -+NOT_YET_IMPLEMENTED_StmtImport - - # fmt: off --import sys -+NOT_YET_IMPLEMENTED_StmtImport - # fmt: on - --import tempfile --import zoneinfo -+NOT_YET_IMPLEMENTED_StmtImport -+NOT_YET_IMPLEMENTED_StmtImport -``` - -## Ruff Output - -```py -# Regression test for https://github.com/psf/black/issues/3438 - -NOT_YET_IMPLEMENTED_StmtImport -NOT_YET_IMPLEMENTED_StmtImport # fmt: skip -NOT_YET_IMPLEMENTED_StmtImport -# fmt: off -NOT_YET_IMPLEMENTED_StmtImport -# fmt: on -NOT_YET_IMPLEMENTED_StmtImport - -NOT_YET_IMPLEMENTED_StmtImport # fmt: skip -NOT_YET_IMPLEMENTED_StmtImport - -# fmt: off -NOT_YET_IMPLEMENTED_StmtImport -# fmt: on - -NOT_YET_IMPLEMENTED_StmtImport -NOT_YET_IMPLEMENTED_StmtImport -``` - -## Black Output - -```py -# Regression test for https://github.com/psf/black/issues/3438 - -import ast -import collections # fmt: skip -import dataclasses -# fmt: off -import os -# fmt: on -import pathlib - -import re # fmt: skip -import secrets - -# fmt: off -import sys -# fmt: on - -import tempfile -import zoneinfo -``` - - diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function.py.snap index 771b8b6398197..3536f09059fcd 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function.py.snap @@ -107,20 +107,12 @@ def __await__(): return (yield) ```diff --- Black +++ Ruff -@@ -1,12 +1,11 @@ - #!/usr/bin/env python3 --import asyncio --import sys -+NOT_YET_IMPLEMENTED_StmtImport -+NOT_YET_IMPLEMENTED_StmtImport +@@ -5,8 +5,7 @@ + from third_party import X, Y, Z --from third_party import X, Y, Z -+NOT_YET_IMPLEMENTED_StmtImportFrom - --from library import some_connection, some_decorator + from library import some_connection, some_decorator - -f"trigger 3.6 mode" -+NOT_YET_IMPLEMENTED_StmtImportFrom +NOT_YET_IMPLEMENTED_ExprJoinedStr @@ -221,12 +213,12 @@ def __await__(): return (yield) ```py #!/usr/bin/env python3 -NOT_YET_IMPLEMENTED_StmtImport -NOT_YET_IMPLEMENTED_StmtImport +import asyncio +import sys -NOT_YET_IMPLEMENTED_StmtImportFrom +from third_party import X, Y, Z -NOT_YET_IMPLEMENTED_StmtImportFrom +from library import some_connection, some_decorator NOT_YET_IMPLEMENTED_ExprJoinedStr diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function2.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function2.py.snap index 3fd47e2167c3f..23da8b63513e6 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function2.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__function2.py.snap @@ -65,22 +65,15 @@ with hmm_but_this_should_get_two_preceding_newlines(): ```diff --- Black +++ Ruff -@@ -32,34 +32,28 @@ - - - if os.name == "posix": -- import termios -+ NOT_YET_IMPLEMENTED_StmtImport +@@ -36,7 +36,6 @@ def i_should_be_followed_by_only_one_newline(): pass - elif os.name == "nt": try: -- import msvcrt -+ NOT_YET_IMPLEMENTED_StmtImport - - def i_should_be_followed_by_only_one_newline(): + import msvcrt +@@ -45,21 +44,16 @@ pass except ImportError: @@ -141,13 +134,13 @@ def h(): if os.name == "posix": - NOT_YET_IMPLEMENTED_StmtImport + import termios def i_should_be_followed_by_only_one_newline(): pass elif os.name == "nt": try: - NOT_YET_IMPLEMENTED_StmtImport + import msvcrt def i_should_be_followed_by_only_one_newline(): pass diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__import_spacing.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__import_spacing.py.snap index 6b2e7df8f35f7..f08a097d01e43 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__import_spacing.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__import_spacing.py.snap @@ -61,79 +61,15 @@ __all__ = ( ```diff --- Black +++ Ruff -@@ -2,53 +2,31 @@ - - # flake8: noqa - --from logging import WARNING --from logging import ( -- ERROR, --) --import sys -+NOT_YET_IMPLEMENTED_StmtImportFrom -+NOT_YET_IMPLEMENTED_StmtImportFrom -+NOT_YET_IMPLEMENTED_StmtImport - - # This relies on each of the submodules having an __all__ variable. --from .base_events import * --from .coroutines import * --from .events import * # comment here -+NOT_YET_IMPLEMENTED_StmtImportFrom -+NOT_YET_IMPLEMENTED_StmtImportFrom -+NOT_YET_IMPLEMENTED_StmtImportFrom # comment here - --from .futures import * --from .locks import * # comment here --from .protocols import * -+NOT_YET_IMPLEMENTED_StmtImportFrom -+NOT_YET_IMPLEMENTED_StmtImportFrom # comment here -+NOT_YET_IMPLEMENTED_StmtImportFrom - --from ..runners import * # comment here --from ..queues import * --from ..streams import * -+NOT_YET_IMPLEMENTED_StmtImportFrom # comment here -+NOT_YET_IMPLEMENTED_StmtImportFrom -+NOT_YET_IMPLEMENTED_StmtImportFrom - --from some_library import ( -- Just, -- Enough, -- Libraries, -- To, -- Fit, -- In, -- This, -- Nice, -- Split, -- Which, -- We, -- No, -- Longer, -- Use, --) --from name_of_a_company.extremely_long_project_name.component.ttypes import ( +@@ -38,7 +38,7 @@ + Use, + ) + from name_of_a_company.extremely_long_project_name.component.ttypes import ( - CuteLittleServiceHandlerFactoryyy, --) --from name_of_a_company.extremely_long_project_name.extremely_long_component_name.ttypes import * -+NOT_YET_IMPLEMENTED_StmtImportFrom -+NOT_YET_IMPLEMENTED_StmtImportFrom -+NOT_YET_IMPLEMENTED_StmtImportFrom - --from .a.b.c.subprocess import * --from . import tasks --from . import A, B, C --from . import ( -- SomeVeryLongNameAndAllOfItsAdditionalLetters1, -- SomeVeryLongNameAndAllOfItsAdditionalLetters2, --) -+NOT_YET_IMPLEMENTED_StmtImportFrom -+NOT_YET_IMPLEMENTED_StmtImportFrom -+NOT_YET_IMPLEMENTED_StmtImportFrom -+NOT_YET_IMPLEMENTED_StmtImportFrom ++ CuteLittleServiceHandlerFactoryyy + ) + from name_of_a_company.extremely_long_project_name.extremely_long_component_name.ttypes import * - __all__ = ( - base_events.__all__ ``` ## Ruff Output @@ -143,31 +79,53 @@ __all__ = ( # flake8: noqa -NOT_YET_IMPLEMENTED_StmtImportFrom -NOT_YET_IMPLEMENTED_StmtImportFrom -NOT_YET_IMPLEMENTED_StmtImport +from logging import WARNING +from logging import ( + ERROR, +) +import sys # This relies on each of the submodules having an __all__ variable. -NOT_YET_IMPLEMENTED_StmtImportFrom -NOT_YET_IMPLEMENTED_StmtImportFrom -NOT_YET_IMPLEMENTED_StmtImportFrom # comment here - -NOT_YET_IMPLEMENTED_StmtImportFrom -NOT_YET_IMPLEMENTED_StmtImportFrom # comment here -NOT_YET_IMPLEMENTED_StmtImportFrom - -NOT_YET_IMPLEMENTED_StmtImportFrom # comment here -NOT_YET_IMPLEMENTED_StmtImportFrom -NOT_YET_IMPLEMENTED_StmtImportFrom - -NOT_YET_IMPLEMENTED_StmtImportFrom -NOT_YET_IMPLEMENTED_StmtImportFrom -NOT_YET_IMPLEMENTED_StmtImportFrom - -NOT_YET_IMPLEMENTED_StmtImportFrom -NOT_YET_IMPLEMENTED_StmtImportFrom -NOT_YET_IMPLEMENTED_StmtImportFrom -NOT_YET_IMPLEMENTED_StmtImportFrom +from .base_events import * +from .coroutines import * +from .events import * # comment here + +from .futures import * +from .locks import * # comment here +from .protocols import * + +from ..runners import * # comment here +from ..queues import * +from ..streams import * + +from some_library import ( + Just, + Enough, + Libraries, + To, + Fit, + In, + This, + Nice, + Split, + Which, + We, + No, + Longer, + Use, +) +from name_of_a_company.extremely_long_project_name.component.ttypes import ( + CuteLittleServiceHandlerFactoryyy +) +from name_of_a_company.extremely_long_project_name.extremely_long_component_name.ttypes import * + +from .a.b.c.subprocess import * +from . import tasks +from . import A, B, C +from . import ( + SomeVeryLongNameAndAllOfItsAdditionalLetters1, + SomeVeryLongNameAndAllOfItsAdditionalLetters2, +) __all__ = ( base_events.__all__ diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__remove_await_parens.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__remove_await_parens.py.snap index e77d6934e304c..d529f18f49088 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__remove_await_parens.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__remove_await_parens.py.snap @@ -93,12 +93,6 @@ async def main(): ```diff --- Black +++ Ruff -@@ -1,4 +1,4 @@ --import asyncio -+NOT_YET_IMPLEMENTED_StmtImport - - - # Control example @@ -8,59 +8,70 @@ # Remove brackets for short coroutine/task @@ -219,7 +213,7 @@ async def main(): ## Ruff Output ```py -NOT_YET_IMPLEMENTED_StmtImport +import asyncio # Control example diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__remove_newline_after_code_block_open.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__remove_newline_after_code_block_open.py.snap index f9095f8b6ff68..c97f2a11d6187 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__remove_newline_after_code_block_open.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__remove_newline_after_code_block_open.py.snap @@ -120,12 +120,6 @@ with open("/path/to/file.txt", mode="r") as read_file: ```diff --- Black +++ Ruff -@@ -1,4 +1,4 @@ --import random -+NOT_YET_IMPLEMENTED_StmtImport - - - def foo1(): @@ -27,16 +27,16 @@ @@ -151,7 +145,7 @@ with open("/path/to/file.txt", mode="r") as read_file: ## Ruff Output ```py -NOT_YET_IMPLEMENTED_StmtImport +import random def foo1(): diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__attribute.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__attribute.py.snap index bbff73657c9a9..2b48c542e5020 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__attribute.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__attribute.py.snap @@ -111,7 +111,7 @@ x53 = ( ## Output ```py -NOT_YET_IMPLEMENTED_StmtImportFrom +from argparse import Namespace a = Namespace() diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap index 408588813cb8b..f8a09bc04e07f 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap @@ -96,7 +96,7 @@ f( ## Output ```py -NOT_YET_IMPLEMENTED_StmtImportFrom +from unittest.mock import MagicMock def f(*args, **kwargs): diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__import.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__import.py.snap new file mode 100644 index 0000000000000..020860638336b --- /dev/null +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__import.py.snap @@ -0,0 +1,28 @@ +--- +source: crates/ruff_python_formatter/tests/fixtures.rs +input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/import.py +--- +## Input +```py +from a import aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa +from a import aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa +from a import aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa as dfgsdfgsd, aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa as sdkjflsdjlahlfd +``` + +## Output +```py +from a import ( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa +) +from a import ( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, +) +from a import ( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa as dfgsdfgsd, + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa as sdkjflsdjlahlfd, +) +``` + + + diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__import_from.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__import_from.py.snap new file mode 100644 index 0000000000000..0d8c90572e987 --- /dev/null +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__import_from.py.snap @@ -0,0 +1,46 @@ +--- +source: crates/ruff_python_formatter/tests/fixtures.rs +input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/import_from.py +--- +## Input +```py +from a import aksjdhflsakhdflkjsadlfajkslhf +from a import ( + aksjdhflsakhdflkjsadlfajkslhf, +) +from a import ( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, +) +from a import ( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, +) +from a import ( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa as dfgsdfgsd, + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa as sdkjflsdjlahlfd, +) +from aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa import * +``` + +## Output +```py +from a import aksjdhflsakhdflkjsadlfajkslhf +from a import ( + aksjdhflsakhdflkjsadlfajkslhf, +) +from a import ( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, +) +from a import ( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa, +) +from a import ( + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa as dfgsdfgsd, + aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa as sdkjflsdjlahlfd, +) +from aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfksajlhfajfjfsaahflakjslhdfkjalhdskjfa import * +``` + + + From 937de121f37e3ff32308bef0ba5a6cb438012ea8 Mon Sep 17 00:00:00 2001 From: konsti Date: Tue, 4 Jul 2023 09:54:35 +0200 Subject: [PATCH 04/15] check-formatter-stability: Remove newlines and add `--error-file` (#5491) ## Summary This makes the output of `check-formatter-stability` more concise by removing extraneous newlines. It also adds a `--error-file` option to that script that allows creating a file with just the errors (without the status messages) to share with others. ## Test Plan I ran it over CPython and looked at the output. I then added the `--error-file` option and looked at the contents of the file --- .../ruff_dev/src/check_formatter_stability.rs | 50 +++++++++++-------- 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/crates/ruff_dev/src/check_formatter_stability.rs b/crates/ruff_dev/src/check_formatter_stability.rs index 95a37e6838286..b955a7245a077 100644 --- a/crates/ruff_dev/src/check_formatter_stability.rs +++ b/crates/ruff_dev/src/check_formatter_stability.rs @@ -4,8 +4,9 @@ //! checking entire repositories. use std::fmt::{Display, Formatter}; -use std::io::stdout; +use std::fs::File; use std::io::Write; +use std::io::{stdout, BufWriter}; use std::panic::catch_unwind; use std::path::{Path, PathBuf}; use std::process::ExitCode; @@ -49,6 +50,9 @@ pub(crate) struct Args { /// Checks each project inside a directory #[arg(long)] pub(crate) multi_project: bool, + /// Write all errors to this file in addition to stdout + #[arg(long)] + pub(crate) error_file: Option, } /// Generate ourself a `try_parse_from` impl for `CheckArgs`. This is a strange way to use clap but @@ -69,6 +73,12 @@ pub(crate) fn main(args: &Args) -> anyhow::Result { #[allow(clippy::print_stdout)] { print!("{}", result.display(args.format)); + println!( + "Found {} stability errors in {} files in {:.2}s", + result.diagnostics.len(), + result.file_count, + result.duration.as_secs_f32(), + ); } result.is_success() @@ -114,6 +124,7 @@ fn check_multi_project(args: &Args) -> bool { match check_repo(&Args { files: vec![path.clone()], + error_file: args.error_file.clone(), ..*args }) { Ok(result) => sender.send(Message::Finished { result, path }), @@ -126,6 +137,9 @@ fn check_multi_project(args: &Args) -> bool { scope.spawn(|_| { let mut stdout = stdout().lock(); + let mut error_file = args.error_file.as_ref().map(|error_file| { + BufWriter::new(File::create(error_file).expect("Couldn't open error file")) + }); for message in receiver { match message { @@ -135,13 +149,19 @@ fn check_multi_project(args: &Args) -> bool { Message::Finished { path, result } => { total_errors += result.diagnostics.len(); total_files += result.file_count; + writeln!( stdout, - "Finished {}\n{}\n", + "Finished {} with {} files in {:.2}s", path.display(), - result.display(args.format) + result.file_count, + result.duration.as_secs_f32(), ) .unwrap(); + write!(stdout, "{}", result.display(args.format)).unwrap(); + if let Some(error_file) = &mut error_file { + write!(error_file, "{}", result.display(args.format)).unwrap(); + } all_success = all_success && result.is_success(); } Message::Failed { path, error } => { @@ -157,8 +177,10 @@ fn check_multi_project(args: &Args) -> bool { #[allow(clippy::print_stdout)] { - println!("{total_errors} stability errors in {total_files} files"); - println!("Finished in {}s", duration.as_secs_f32()); + println!( + "{total_errors} stability errors in {total_files} files in {}s", + duration.as_secs_f32() + ); } all_success @@ -295,23 +317,11 @@ struct DisplayCheckRepoResult<'a> { } impl Display for DisplayCheckRepoResult<'_> { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - let CheckRepoResult { - duration, - file_count, - diagnostics, - } = self.result; - - for diagnostic in diagnostics { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + for diagnostic in &self.result.diagnostics { write!(f, "{}", diagnostic.display(self.format))?; } - - writeln!( - f, - "Formatting {} files twice took {:.2}s", - file_count, - duration.as_secs_f32() - ) + Ok(()) } } From 0b963ddcfa4e9f7c69103770ea2050117d4314fb Mon Sep 17 00:00:00 2001 From: Thomas de Zeeuw Date: Tue, 4 Jul 2023 16:27:23 +0200 Subject: [PATCH 05/15] Add unreachable code rule (#5384) Co-authored-by: Thomas de Zeeuw Co-authored-by: Micha Reiser --- .gitattributes | 1 + Cargo.lock | 1 + crates/ruff/Cargo.toml | 3 + .../fixtures/control-flow-graph/assert.py | 11 + .../fixtures/control-flow-graph/async-for.py | 41 + .../test/fixtures/control-flow-graph/for.py | 41 + .../test/fixtures/control-flow-graph/if.py | 108 ++ .../test/fixtures/control-flow-graph/match.py | 131 ++ .../test/fixtures/control-flow-graph/raise.py | 5 + .../fixtures/control-flow-graph/simple.py | 23 + .../test/fixtures/control-flow-graph/try.py | 41 + .../test/fixtures/control-flow-graph/while.py | 121 ++ .../resources/test/fixtures/ruff/RUF014.py | 185 +++ crates/ruff/src/checkers/ast/mod.rs | 5 + crates/ruff/src/codes.rs | 2 + crates/ruff/src/rules/ruff/mod.rs | 4 + crates/ruff/src/rules/ruff/rules/mod.rs | 4 + ...les__unreachable__tests__assert.py.md.snap | 97 ++ ...__unreachable__tests__async-for.py.md.snap | 241 ++++ ..._rules__unreachable__tests__for.py.md.snap | 241 ++++ ...__rules__unreachable__tests__if.py.md.snap | 535 ++++++++ ...ules__unreachable__tests__match.py.md.snap | 776 ++++++++++++ ...ules__unreachable__tests__raise.py.md.snap | 41 + ...les__unreachable__tests__simple.py.md.snap | 136 ++ ...ules__unreachable__tests__while.py.md.snap | 527 ++++++++ .../ruff/src/rules/ruff/rules/unreachable.rs | 1101 +++++++++++++++++ ..._rules__ruff__tests__RUF014_RUF014.py.snap | 249 ++++ crates/ruff_dev/src/generate_json_schema.rs | 2 +- crates/ruff_index/src/slice.rs | 12 + crates/ruff_macros/src/newtype_index.rs | 7 +- 30 files changed, 4688 insertions(+), 4 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/control-flow-graph/assert.py create mode 100644 crates/ruff/resources/test/fixtures/control-flow-graph/async-for.py create mode 100644 crates/ruff/resources/test/fixtures/control-flow-graph/for.py create mode 100644 crates/ruff/resources/test/fixtures/control-flow-graph/if.py create mode 100644 crates/ruff/resources/test/fixtures/control-flow-graph/match.py create mode 100644 crates/ruff/resources/test/fixtures/control-flow-graph/raise.py create mode 100644 crates/ruff/resources/test/fixtures/control-flow-graph/simple.py create mode 100644 crates/ruff/resources/test/fixtures/control-flow-graph/try.py create mode 100644 crates/ruff/resources/test/fixtures/control-flow-graph/while.py create mode 100644 crates/ruff/resources/test/fixtures/ruff/RUF014.py create mode 100644 crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__assert.py.md.snap create mode 100644 crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__async-for.py.md.snap create mode 100644 crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__for.py.md.snap create mode 100644 crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__if.py.md.snap create mode 100644 crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__match.py.md.snap create mode 100644 crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__raise.py.md.snap create mode 100644 crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__simple.py.md.snap create mode 100644 crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__while.py.md.snap create mode 100644 crates/ruff/src/rules/ruff/rules/unreachable.rs create mode 100644 crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF014_RUF014.py.snap diff --git a/.gitattributes b/.gitattributes index 5bb8d8b736ced..c4b5fa0751be8 100644 --- a/.gitattributes +++ b/.gitattributes @@ -4,3 +4,4 @@ crates/ruff/resources/test/fixtures/isort/line_ending_crlf.py text eol=crlf crates/ruff/resources/test/fixtures/pycodestyle/W605_1.py text eol=crlf ruff.schema.json linguist-generated=true text=auto eol=lf +*.md.snap linguist-language=Markdown diff --git a/Cargo.lock b/Cargo.lock index ca1ef092902dd..efc811ad0e934 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1865,6 +1865,7 @@ dependencies = [ "result-like", "ruff_cache", "ruff_diagnostics", + "ruff_index", "ruff_macros", "ruff_python_ast", "ruff_python_semantic", diff --git a/crates/ruff/Cargo.toml b/crates/ruff/Cargo.toml index 350af4bebc7cb..c7a401b79cde5 100644 --- a/crates/ruff/Cargo.toml +++ b/crates/ruff/Cargo.toml @@ -17,6 +17,7 @@ name = "ruff" [dependencies] ruff_cache = { path = "../ruff_cache" } ruff_diagnostics = { path = "../ruff_diagnostics", features = ["serde"] } +ruff_index = { path = "../ruff_index" } ruff_macros = { path = "../ruff_macros" } ruff_python_whitespace = { path = "../ruff_python_whitespace" } ruff_python_ast = { path = "../ruff_python_ast", features = ["serde"] } @@ -88,3 +89,5 @@ colored = { workspace = true, features = ["no-color"] } [features] default = [] schemars = ["dep:schemars"] +# Enables the UnreachableCode rule +unreachable-code = [] diff --git a/crates/ruff/resources/test/fixtures/control-flow-graph/assert.py b/crates/ruff/resources/test/fixtures/control-flow-graph/assert.py new file mode 100644 index 0000000000000..bfb3ab9030e90 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/control-flow-graph/assert.py @@ -0,0 +1,11 @@ +def func(): + assert True + +def func(): + assert False + +def func(): + assert True, "oops" + +def func(): + assert False, "oops" diff --git a/crates/ruff/resources/test/fixtures/control-flow-graph/async-for.py b/crates/ruff/resources/test/fixtures/control-flow-graph/async-for.py new file mode 100644 index 0000000000000..a1dc86a6e93d1 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/control-flow-graph/async-for.py @@ -0,0 +1,41 @@ +def func(): + async for i in range(5): + print(i) + +def func(): + async for i in range(20): + print(i) + else: + return 0 + +def func(): + async for i in range(10): + if i == 5: + return 1 + return 0 + +def func(): + async for i in range(111): + if i == 5: + return 1 + else: + return 0 + return 2 + +def func(): + async for i in range(12): + continue + +def func(): + async for i in range(1110): + if True: + continue + +def func(): + async for i in range(13): + break + +def func(): + async for i in range(1110): + if True: + break diff --git a/crates/ruff/resources/test/fixtures/control-flow-graph/for.py b/crates/ruff/resources/test/fixtures/control-flow-graph/for.py new file mode 100644 index 0000000000000..a5807a635a412 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/control-flow-graph/for.py @@ -0,0 +1,41 @@ +def func(): + for i in range(5): + print(i) + +def func(): + for i in range(20): + print(i) + else: + return 0 + +def func(): + for i in range(10): + if i == 5: + return 1 + return 0 + +def func(): + for i in range(111): + if i == 5: + return 1 + else: + return 0 + return 2 + +def func(): + for i in range(12): + continue + +def func(): + for i in range(1110): + if True: + continue + +def func(): + for i in range(13): + break + +def func(): + for i in range(1110): + if True: + break diff --git a/crates/ruff/resources/test/fixtures/control-flow-graph/if.py b/crates/ruff/resources/test/fixtures/control-flow-graph/if.py new file mode 100644 index 0000000000000..2b5fa420990ec --- /dev/null +++ b/crates/ruff/resources/test/fixtures/control-flow-graph/if.py @@ -0,0 +1,108 @@ +def func(): + if False: + return 0 + return 1 + +def func(): + if True: + return 1 + return 0 + +def func(): + if False: + return 0 + else: + return 1 + +def func(): + if True: + return 1 + else: + return 0 + +def func(): + if False: + return 0 + else: + return 1 + return "unreachable" + +def func(): + if True: + return 1 + else: + return 0 + return "unreachable" + +def func(): + if True: + if True: + return 1 + return 2 + else: + return 3 + return "unreachable2" + +def func(): + if False: + return 0 + +def func(): + if True: + return 1 + +def func(): + if True: + return 1 + elif False: + return 2 + else: + return 0 + +def func(): + if False: + return 1 + elif True: + return 2 + else: + return 0 + +def func(): + if True: + if False: + return 0 + elif True: + return 1 + else: + return 2 + return 3 + elif True: + return 4 + else: + return 5 + return 6 + +def func(): + if False: + return "unreached" + elif False: + return "also unreached" + return "reached" + +# Test case found in the Bokeh repository that trigger a false positive. +def func(self, obj: BytesRep) -> bytes: + data = obj["data"] + + if isinstance(data, str): + return base64.b64decode(data) + elif isinstance(data, Buffer): + buffer = data + else: + id = data["id"] + + if id in self._buffers: + buffer = self._buffers[id] + else: + self.error(f"can't resolve buffer '{id}'") + + return buffer.data diff --git a/crates/ruff/resources/test/fixtures/control-flow-graph/match.py b/crates/ruff/resources/test/fixtures/control-flow-graph/match.py new file mode 100644 index 0000000000000..cce019e3085e9 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/control-flow-graph/match.py @@ -0,0 +1,131 @@ +def func(status): + match status: + case _: + return 0 + return "unreachable" + +def func(status): + match status: + case 1: + return 1 + return 0 + +def func(status): + match status: + case 1: + return 1 + case _: + return 0 + +def func(status): + match status: + case 1 | 2 | 3: + return 5 + return 6 + +def func(status): + match status: + case 1 | 2 | 3: + return 5 + case _: + return 10 + return 0 + +def func(status): + match status: + case 0: + return 0 + case 1: + return 1 + case 1: + return "1 again" + case _: + return 3 + +def func(status): + i = 0 + match status, i: + case _, _: + return 0 + +def func(status): + i = 0 + match status, i: + case _, 0: + return 0 + case _, 2: + return 0 + +def func(point): + match point: + case (0, 0): + print("Origin") + case _: + raise ValueError("oops") + +def func(point): + match point: + case (0, 0): + print("Origin") + case (0, y): + print(f"Y={y}") + case (x, 0): + print(f"X={x}") + case (x, y): + print(f"X={x}, Y={y}") + case _: + raise ValueError("Not a point") + +def where_is(point): + class Point: + x: int + y: int + + match point: + case Point(x=0, y=0): + print("Origin") + case Point(x=0, y=y): + print(f"Y={y}") + case Point(x=x, y=0): + print(f"X={x}") + case Point(): + print("Somewhere else") + case _: + print("Not a point") + +def func(points): + match points: + case []: + print("No points") + case [Point(0, 0)]: + print("The origin") + case [Point(x, y)]: + print(f"Single point {x}, {y}") + case [Point(0, y1), Point(0, y2)]: + print(f"Two on the Y axis at {y1}, {y2}") + case _: + print("Something else") + +def func(point): + match point: + case Point(x, y) if x == y: + print(f"Y=X at {x}") + case Point(x, y): + print(f"Not on the diagonal") + +def func(): + from enum import Enum + class Color(Enum): + RED = 'red' + GREEN = 'green' + BLUE = 'blue' + + color = Color(input("Enter your choice of 'red', 'blue' or 'green': ")) + + match color: + case Color.RED: + print("I see red!") + case Color.GREEN: + print("Grass is green") + case Color.BLUE: + print("I'm feeling the blues :(") diff --git a/crates/ruff/resources/test/fixtures/control-flow-graph/raise.py b/crates/ruff/resources/test/fixtures/control-flow-graph/raise.py new file mode 100644 index 0000000000000..37aadc61a0460 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/control-flow-graph/raise.py @@ -0,0 +1,5 @@ +def func(): + raise Exception + +def func(): + raise "a glass!" diff --git a/crates/ruff/resources/test/fixtures/control-flow-graph/simple.py b/crates/ruff/resources/test/fixtures/control-flow-graph/simple.py new file mode 100644 index 0000000000000..d1f710149b627 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/control-flow-graph/simple.py @@ -0,0 +1,23 @@ +def func(): + pass + +def func(): + pass + +def func(): + return + +def func(): + return 1 + +def func(): + return 1 + return "unreachable" + +def func(): + i = 0 + +def func(): + i = 0 + i += 2 + return i diff --git a/crates/ruff/resources/test/fixtures/control-flow-graph/try.py b/crates/ruff/resources/test/fixtures/control-flow-graph/try.py new file mode 100644 index 0000000000000..e9f109dfd7bf6 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/control-flow-graph/try.py @@ -0,0 +1,41 @@ +def func(): + try: + ... + except Exception: + ... + except OtherException as e: + ... + else: + ... + finally: + ... + +def func(): + try: + ... + except Exception: + ... + +def func(): + try: + ... + except Exception: + ... + except OtherException as e: + ... + +def func(): + try: + ... + except Exception: + ... + except OtherException as e: + ... + else: + ... + +def func(): + try: + ... + finally: + ... diff --git a/crates/ruff/resources/test/fixtures/control-flow-graph/while.py b/crates/ruff/resources/test/fixtures/control-flow-graph/while.py new file mode 100644 index 0000000000000..6a4174358bdba --- /dev/null +++ b/crates/ruff/resources/test/fixtures/control-flow-graph/while.py @@ -0,0 +1,121 @@ +def func(): + while False: + return "unreachable" + return 1 + +def func(): + while False: + return "unreachable" + else: + return 1 + +def func(): + while False: + return "unreachable" + else: + return 1 + return "also unreachable" + +def func(): + while True: + return 1 + return "unreachable" + +def func(): + while True: + return 1 + else: + return "unreachable" + +def func(): + while True: + return 1 + else: + return "unreachable" + return "also unreachable" + +def func(): + i = 0 + while False: + i += 1 + return i + +def func(): + i = 0 + while True: + i += 1 + return i + +def func(): + while True: + pass + return 1 + +def func(): + i = 0 + while True: + if True: + print("ok") + i += 1 + return i + +def func(): + i = 0 + while True: + if False: + print("ok") + i += 1 + return i + +def func(): + while True: + if True: + return 1 + return 0 + +def func(): + while True: + continue + +def func(): + while False: + continue + +def func(): + while True: + break + +def func(): + while False: + break + +def func(): + while True: + if True: + continue + +def func(): + while True: + if True: + break + +''' +TODO: because `try` statements aren't handled this triggers a false positive as +the last statement is reached, but the rules thinks it isn't (it doesn't +see/process the break statement). + +# Test case found in the Bokeh repository that trigger a false positive. +def bokeh2(self, host: str = DEFAULT_HOST, port: int = DEFAULT_PORT) -> None: + self.stop_serving = False + while True: + try: + self.server = HTTPServer((host, port), HtmlOnlyHandler) + self.host = host + self.port = port + break + except OSError: + log.debug(f"port {port} is in use, trying to next one") + port += 1 + + self.thread = threading.Thread(target=self._run_web_server) +''' diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF014.py b/crates/ruff/resources/test/fixtures/ruff/RUF014.py new file mode 100644 index 0000000000000..d1ae40f3ca864 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/ruff/RUF014.py @@ -0,0 +1,185 @@ +def after_return(): + return "reachable" + return "unreachable" + +async def also_works_on_async_functions(): + return "reachable" + return "unreachable" + +def if_always_true(): + if True: + return "reachable" + return "unreachable" + +def if_always_false(): + if False: + return "unreachable" + return "reachable" + +def if_elif_always_false(): + if False: + return "unreachable" + elif False: + return "also unreachable" + return "reachable" + +def if_elif_always_true(): + if False: + return "unreachable" + elif True: + return "reachable" + return "also unreachable" + +def ends_with_if(): + if False: + return "unreachable" + else: + return "reachable" + +def infinite_loop(): + while True: + continue + return "unreachable" + +''' TODO: we could determine these, but we don't yet. +def for_range_return(): + for i in range(10): + if i == 5: + return "reachable" + return "unreachable" + +def for_range_else(): + for i in range(111): + if i == 5: + return "reachable" + else: + return "unreachable" + return "also unreachable" + +def for_range_break(): + for i in range(13): + return "reachable" + return "unreachable" + +def for_range_if_break(): + for i in range(1110): + if True: + return "reachable" + return "unreachable" +''' + +def match_wildcard(status): + match status: + case _: + return "reachable" + return "unreachable" + +def match_case_and_wildcard(status): + match status: + case 1: + return "reachable" + case _: + return "reachable" + return "unreachable" + +def raise_exception(): + raise Exception + return "unreachable" + +def while_false(): + while False: + return "unreachable" + return "reachable" + +def while_false_else(): + while False: + return "unreachable" + else: + return "reachable" + +def while_false_else_return(): + while False: + return "unreachable" + else: + return "reachable" + return "also unreachable" + +def while_true(): + while True: + return "reachable" + return "unreachable" + +def while_true_else(): + while True: + return "reachable" + else: + return "unreachable" + +def while_true_else_return(): + while True: + return "reachable" + else: + return "unreachable" + return "also unreachable" + +def while_false_var_i(): + i = 0 + while False: + i += 1 + return i + +def while_true_var_i(): + i = 0 + while True: + i += 1 + return i + +def while_infinite(): + while True: + pass + return "unreachable" + +def while_if_true(): + while True: + if True: + return "reachable" + return "unreachable" + +# Test case found in the Bokeh repository that trigger a false positive. +def bokeh1(self, obj: BytesRep) -> bytes: + data = obj["data"] + + if isinstance(data, str): + return base64.b64decode(data) + elif isinstance(data, Buffer): + buffer = data + else: + id = data["id"] + + if id in self._buffers: + buffer = self._buffers[id] + else: + self.error(f"can't resolve buffer '{id}'") + + return buffer.data + +''' +TODO: because `try` statements aren't handled this triggers a false positive as +the last statement is reached, but the rules thinks it isn't (it doesn't +see/process the break statement). + +# Test case found in the Bokeh repository that trigger a false positive. +def bokeh2(self, host: str = DEFAULT_HOST, port: int = DEFAULT_PORT) -> None: + self.stop_serving = False + while True: + try: + self.server = HTTPServer((host, port), HtmlOnlyHandler) + self.host = host + self.port = port + break + except OSError: + log.debug(f"port {port} is in use, trying to next one") + port += 1 + + self.thread = threading.Thread(target=self._run_web_server) +''' diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 6eb1f6b25cec1..c4f3617120aa5 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -619,6 +619,11 @@ where ); } } + #[cfg(feature = "unreachable-code")] + if self.enabled(Rule::UnreachableCode) { + self.diagnostics + .extend(ruff::rules::unreachable::in_function(name, body)); + } } Stmt::Return(_) => { if self.enabled(Rule::ReturnOutsideFunction) { diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 79a7982dc9c8f..5e526f599dbdd 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -761,6 +761,8 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "011") => (RuleGroup::Unspecified, rules::ruff::rules::StaticKeyDictComprehension), (Ruff, "012") => (RuleGroup::Unspecified, rules::ruff::rules::MutableClassDefault), (Ruff, "013") => (RuleGroup::Unspecified, rules::ruff::rules::ImplicitOptional), + #[cfg(feature = "unreachable-code")] + (Ruff, "014") => (RuleGroup::Nursery, rules::ruff::rules::UnreachableCode), (Ruff, "100") => (RuleGroup::Unspecified, rules::ruff::rules::UnusedNOQA), (Ruff, "200") => (RuleGroup::Unspecified, rules::ruff::rules::InvalidPyprojectToml), diff --git a/crates/ruff/src/rules/ruff/mod.rs b/crates/ruff/src/rules/ruff/mod.rs index f0aa41c57043a..a110328a248f7 100644 --- a/crates/ruff/src/rules/ruff/mod.rs +++ b/crates/ruff/src/rules/ruff/mod.rs @@ -30,6 +30,10 @@ mod tests { #[test_case(Rule::MutableDataclassDefault, Path::new("RUF008.py"))] #[test_case(Rule::PairwiseOverZipped, Path::new("RUF007.py"))] #[test_case(Rule::StaticKeyDictComprehension, Path::new("RUF011.py"))] + #[cfg_attr( + feature = "unreachable-code", + test_case(Rule::UnreachableCode, Path::new("RUF014.py")) + )] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff/src/rules/ruff/rules/mod.rs b/crates/ruff/src/rules/ruff/rules/mod.rs index 6ec0eda5906c6..f79f5fafd7652 100644 --- a/crates/ruff/src/rules/ruff/rules/mod.rs +++ b/crates/ruff/src/rules/ruff/rules/mod.rs @@ -9,6 +9,8 @@ pub(crate) use mutable_class_default::*; pub(crate) use mutable_dataclass_default::*; pub(crate) use pairwise_over_zipped::*; pub(crate) use static_key_dict_comprehension::*; +#[cfg(feature = "unreachable-code")] +pub(crate) use unreachable::*; pub(crate) use unused_noqa::*; mod ambiguous_unicode_character; @@ -24,6 +26,8 @@ mod mutable_class_default; mod mutable_dataclass_default; mod pairwise_over_zipped; mod static_key_dict_comprehension; +#[cfg(feature = "unreachable-code")] +pub(crate) mod unreachable; mod unused_noqa; #[derive(Clone, Copy)] diff --git a/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__assert.py.md.snap b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__assert.py.md.snap new file mode 100644 index 0000000000000..17ca4671a0f27 --- /dev/null +++ b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__assert.py.md.snap @@ -0,0 +1,97 @@ +--- +source: crates/ruff/src/rules/ruff/rules/unreachable.rs +description: "This is a Mermaid graph. You can use https://mermaid.live to visualize it as a diagram." +--- +## Function 0 +### Source +```python +def func(): + assert True +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1[["Exception raised"]] + block2["assert True\n"] + + start --> block2 + block2 -- "True" --> block0 + block2 -- "else" --> block1 + block1 --> return + block0 --> return +``` + +## Function 1 +### Source +```python +def func(): + assert False +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1[["Exception raised"]] + block2["assert False\n"] + + start --> block2 + block2 -- "False" --> block0 + block2 -- "else" --> block1 + block1 --> return + block0 --> return +``` + +## Function 2 +### Source +```python +def func(): + assert True, "oops" +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1[["Exception raised"]] + block2["assert True, #quot;oops#quot;\n"] + + start --> block2 + block2 -- "True" --> block0 + block2 -- "else" --> block1 + block1 --> return + block0 --> return +``` + +## Function 3 +### Source +```python +def func(): + assert False, "oops" +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1[["Exception raised"]] + block2["assert False, #quot;oops#quot;\n"] + + start --> block2 + block2 -- "False" --> block0 + block2 -- "else" --> block1 + block1 --> return + block0 --> return +``` + + diff --git a/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__async-for.py.md.snap b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__async-for.py.md.snap new file mode 100644 index 0000000000000..431c82d33c4ad --- /dev/null +++ b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__async-for.py.md.snap @@ -0,0 +1,241 @@ +--- +source: crates/ruff/src/rules/ruff/rules/unreachable.rs +description: "This is a Mermaid graph. You can use https://mermaid.live to visualize it as a diagram." +--- +## Function 0 +### Source +```python +def func(): + async for i in range(5): + print(i) +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["print(i)\n"] + block2["async for i in range(5): + print(i)\n"] + + start --> block2 + block2 -- "range(5)" --> block1 + block2 -- "else" --> block0 + block1 --> block2 + block0 --> return +``` + +## Function 1 +### Source +```python +def func(): + async for i in range(20): + print(i) + else: + return 0 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["print(i)\n"] + block1["return 0\n"] + block2["async for i in range(20): + print(i) + else: + return 0\n"] + + start --> block2 + block2 -- "range(20)" --> block0 + block2 -- "else" --> block1 + block1 --> return + block0 --> return +``` + +## Function 2 +### Source +```python +def func(): + async for i in range(10): + if i == 5: + return 1 + return 0 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 0\n"] + block1["return 1\n"] + block2["if i == 5: + return 1\n"] + block3["async for i in range(10): + if i == 5: + return 1\n"] + + start --> block3 + block3 -- "range(10)" --> block2 + block3 -- "else" --> block0 + block2 -- "i == 5" --> block1 + block2 -- "else" --> block3 + block1 --> return + block0 --> return +``` + +## Function 3 +### Source +```python +def func(): + async for i in range(111): + if i == 5: + return 1 + else: + return 0 + return 2 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 2\n"] + block1["return 1\n"] + block2["if i == 5: + return 1\n"] + block3["return 0\n"] + block4["async for i in range(111): + if i == 5: + return 1 + else: + return 0\n"] + + start --> block4 + block4 -- "range(111)" --> block2 + block4 -- "else" --> block3 + block3 --> return + block2 -- "i == 5" --> block1 + block2 -- "else" --> block4 + block1 --> return + block0 --> return +``` + +## Function 4 +### Source +```python +def func(): + async for i in range(12): + continue +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["continue\n"] + block2["async for i in range(12): + continue\n"] + + start --> block2 + block2 -- "range(12)" --> block1 + block2 -- "else" --> block0 + block1 --> block2 + block0 --> return +``` + +## Function 5 +### Source +```python +def func(): + async for i in range(1110): + if True: + continue +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["continue\n"] + block2["if True: + continue\n"] + block3["async for i in range(1110): + if True: + continue\n"] + + start --> block3 + block3 -- "range(1110)" --> block2 + block3 -- "else" --> block0 + block2 -- "True" --> block1 + block2 -- "else" --> block3 + block1 --> block3 + block0 --> return +``` + +## Function 6 +### Source +```python +def func(): + async for i in range(13): + break +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["break\n"] + block2["async for i in range(13): + break\n"] + + start --> block2 + block2 -- "range(13)" --> block1 + block2 -- "else" --> block0 + block1 --> block0 + block0 --> return +``` + +## Function 7 +### Source +```python +def func(): + async for i in range(1110): + if True: + break +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["break\n"] + block2["if True: + break\n"] + block3["async for i in range(1110): + if True: + break\n"] + + start --> block3 + block3 -- "range(1110)" --> block2 + block3 -- "else" --> block0 + block2 -- "True" --> block1 + block2 -- "else" --> block3 + block1 --> block0 + block0 --> return +``` + + diff --git a/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__for.py.md.snap b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__for.py.md.snap new file mode 100644 index 0000000000000..f3d3def743826 --- /dev/null +++ b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__for.py.md.snap @@ -0,0 +1,241 @@ +--- +source: crates/ruff/src/rules/ruff/rules/unreachable.rs +description: "This is a Mermaid graph. You can use https://mermaid.live to visualize it as a diagram." +--- +## Function 0 +### Source +```python +def func(): + for i in range(5): + print(i) +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["print(i)\n"] + block2["for i in range(5): + print(i)\n"] + + start --> block2 + block2 -- "range(5)" --> block1 + block2 -- "else" --> block0 + block1 --> block2 + block0 --> return +``` + +## Function 1 +### Source +```python +def func(): + for i in range(20): + print(i) + else: + return 0 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["print(i)\n"] + block1["return 0\n"] + block2["for i in range(20): + print(i) + else: + return 0\n"] + + start --> block2 + block2 -- "range(20)" --> block0 + block2 -- "else" --> block1 + block1 --> return + block0 --> return +``` + +## Function 2 +### Source +```python +def func(): + for i in range(10): + if i == 5: + return 1 + return 0 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 0\n"] + block1["return 1\n"] + block2["if i == 5: + return 1\n"] + block3["for i in range(10): + if i == 5: + return 1\n"] + + start --> block3 + block3 -- "range(10)" --> block2 + block3 -- "else" --> block0 + block2 -- "i == 5" --> block1 + block2 -- "else" --> block3 + block1 --> return + block0 --> return +``` + +## Function 3 +### Source +```python +def func(): + for i in range(111): + if i == 5: + return 1 + else: + return 0 + return 2 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 2\n"] + block1["return 1\n"] + block2["if i == 5: + return 1\n"] + block3["return 0\n"] + block4["for i in range(111): + if i == 5: + return 1 + else: + return 0\n"] + + start --> block4 + block4 -- "range(111)" --> block2 + block4 -- "else" --> block3 + block3 --> return + block2 -- "i == 5" --> block1 + block2 -- "else" --> block4 + block1 --> return + block0 --> return +``` + +## Function 4 +### Source +```python +def func(): + for i in range(12): + continue +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["continue\n"] + block2["for i in range(12): + continue\n"] + + start --> block2 + block2 -- "range(12)" --> block1 + block2 -- "else" --> block0 + block1 --> block2 + block0 --> return +``` + +## Function 5 +### Source +```python +def func(): + for i in range(1110): + if True: + continue +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["continue\n"] + block2["if True: + continue\n"] + block3["for i in range(1110): + if True: + continue\n"] + + start --> block3 + block3 -- "range(1110)" --> block2 + block3 -- "else" --> block0 + block2 -- "True" --> block1 + block2 -- "else" --> block3 + block1 --> block3 + block0 --> return +``` + +## Function 6 +### Source +```python +def func(): + for i in range(13): + break +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["break\n"] + block2["for i in range(13): + break\n"] + + start --> block2 + block2 -- "range(13)" --> block1 + block2 -- "else" --> block0 + block1 --> block0 + block0 --> return +``` + +## Function 7 +### Source +```python +def func(): + for i in range(1110): + if True: + break +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["break\n"] + block2["if True: + break\n"] + block3["for i in range(1110): + if True: + break\n"] + + start --> block3 + block3 -- "range(1110)" --> block2 + block3 -- "else" --> block0 + block2 -- "True" --> block1 + block2 -- "else" --> block3 + block1 --> block0 + block0 --> return +``` + + diff --git a/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__if.py.md.snap b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__if.py.md.snap new file mode 100644 index 0000000000000..1ab3a11c7dc42 --- /dev/null +++ b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__if.py.md.snap @@ -0,0 +1,535 @@ +--- +source: crates/ruff/src/rules/ruff/rules/unreachable.rs +description: "This is a Mermaid graph. You can use https://mermaid.live to visualize it as a diagram." +--- +## Function 0 +### Source +```python +def func(): + if False: + return 0 + return 1 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 1\n"] + block1["return 0\n"] + block2["if False: + return 0\n"] + + start --> block2 + block2 -- "False" --> block1 + block2 -- "else" --> block0 + block1 --> return + block0 --> return +``` + +## Function 1 +### Source +```python +def func(): + if True: + return 1 + return 0 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 0\n"] + block1["return 1\n"] + block2["if True: + return 1\n"] + + start --> block2 + block2 -- "True" --> block1 + block2 -- "else" --> block0 + block1 --> return + block0 --> return +``` + +## Function 2 +### Source +```python +def func(): + if False: + return 0 + else: + return 1 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 0\n"] + block1["return 1\n"] + block2["if False: + return 0 + else: + return 1\n"] + + start --> block2 + block2 -- "False" --> block0 + block2 -- "else" --> block1 + block1 --> return + block0 --> return +``` + +## Function 3 +### Source +```python +def func(): + if True: + return 1 + else: + return 0 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 1\n"] + block1["return 0\n"] + block2["if True: + return 1 + else: + return 0\n"] + + start --> block2 + block2 -- "True" --> block0 + block2 -- "else" --> block1 + block1 --> return + block0 --> return +``` + +## Function 4 +### Source +```python +def func(): + if False: + return 0 + else: + return 1 + return "unreachable" +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return #quot;unreachable#quot;\n"] + block1["return 0\n"] + block2["return 1\n"] + block3["if False: + return 0 + else: + return 1\n"] + + start --> block3 + block3 -- "False" --> block1 + block3 -- "else" --> block2 + block2 --> return + block1 --> return + block0 --> return +``` + +## Function 5 +### Source +```python +def func(): + if True: + return 1 + else: + return 0 + return "unreachable" +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return #quot;unreachable#quot;\n"] + block1["return 1\n"] + block2["return 0\n"] + block3["if True: + return 1 + else: + return 0\n"] + + start --> block3 + block3 -- "True" --> block1 + block3 -- "else" --> block2 + block2 --> return + block1 --> return + block0 --> return +``` + +## Function 6 +### Source +```python +def func(): + if True: + if True: + return 1 + return 2 + else: + return 3 + return "unreachable2" +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return #quot;unreachable2#quot;\n"] + block1["return 2\n"] + block2["return 1\n"] + block3["if True: + return 1\n"] + block4["return 3\n"] + block5["if True: + if True: + return 1 + return 2 + else: + return 3\n"] + + start --> block5 + block5 -- "True" --> block3 + block5 -- "else" --> block4 + block4 --> return + block3 -- "True" --> block2 + block3 -- "else" --> block1 + block2 --> return + block1 --> return + block0 --> return +``` + +## Function 7 +### Source +```python +def func(): + if False: + return 0 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["return 0\n"] + block2["if False: + return 0\n"] + + start --> block2 + block2 -- "False" --> block1 + block2 -- "else" --> block0 + block1 --> return + block0 --> return +``` + +## Function 8 +### Source +```python +def func(): + if True: + return 1 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["return 1\n"] + block2["if True: + return 1\n"] + + start --> block2 + block2 -- "True" --> block1 + block2 -- "else" --> block0 + block1 --> return + block0 --> return +``` + +## Function 9 +### Source +```python +def func(): + if True: + return 1 + elif False: + return 2 + else: + return 0 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 1\n"] + block1["return 2\n"] + block2["return 0\n"] + block3["elif False: + return 2 + else: + return 0\n"] + block4["if True: + return 1 + elif False: + return 2 + else: + return 0\n"] + + start --> block4 + block4 -- "True" --> block0 + block4 -- "else" --> block3 + block3 -- "False" --> block1 + block3 -- "else" --> block2 + block2 --> return + block1 --> return + block0 --> return +``` + +## Function 10 +### Source +```python +def func(): + if False: + return 1 + elif True: + return 2 + else: + return 0 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 1\n"] + block1["return 2\n"] + block2["return 0\n"] + block3["elif True: + return 2 + else: + return 0\n"] + block4["if False: + return 1 + elif True: + return 2 + else: + return 0\n"] + + start --> block4 + block4 -- "False" --> block0 + block4 -- "else" --> block3 + block3 -- "True" --> block1 + block3 -- "else" --> block2 + block2 --> return + block1 --> return + block0 --> return +``` + +## Function 11 +### Source +```python +def func(): + if True: + if False: + return 0 + elif True: + return 1 + else: + return 2 + return 3 + elif True: + return 4 + else: + return 5 + return 6 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 6\n"] + block1["return 3\n"] + block2["return 0\n"] + block3["return 1\n"] + block4["return 2\n"] + block5["elif True: + return 1 + else: + return 2\n"] + block6["if False: + return 0 + elif True: + return 1 + else: + return 2\n"] + block7["return 4\n"] + block8["return 5\n"] + block9["elif True: + return 4 + else: + return 5\n"] + block10["if True: + if False: + return 0 + elif True: + return 1 + else: + return 2 + return 3 + elif True: + return 4 + else: + return 5\n"] + + start --> block10 + block10 -- "True" --> block6 + block10 -- "else" --> block9 + block9 -- "True" --> block7 + block9 -- "else" --> block8 + block8 --> return + block7 --> return + block6 -- "False" --> block2 + block6 -- "else" --> block5 + block5 -- "True" --> block3 + block5 -- "else" --> block4 + block4 --> return + block3 --> return + block2 --> return + block1 --> return + block0 --> return +``` + +## Function 12 +### Source +```python +def func(): + if False: + return "unreached" + elif False: + return "also unreached" + return "reached" +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return #quot;reached#quot;\n"] + block1["return #quot;unreached#quot;\n"] + block2["return #quot;also unreached#quot;\n"] + block3["elif False: + return #quot;also unreached#quot;\n"] + block4["if False: + return #quot;unreached#quot; + elif False: + return #quot;also unreached#quot;\n"] + + start --> block4 + block4 -- "False" --> block1 + block4 -- "else" --> block3 + block3 -- "False" --> block2 + block3 -- "else" --> block0 + block2 --> return + block1 --> return + block0 --> return +``` + +## Function 13 +### Source +```python +def func(self, obj: BytesRep) -> bytes: + data = obj["data"] + + if isinstance(data, str): + return base64.b64decode(data) + elif isinstance(data, Buffer): + buffer = data + else: + id = data["id"] + + if id in self._buffers: + buffer = self._buffers[id] + else: + self.error(f"can't resolve buffer '{id}'") + + return buffer.data +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return buffer.data\n"] + block1["return base64.b64decode(data)\n"] + block2["buffer = data\n"] + block3["buffer = self._buffers[id]\n"] + block4["self.error(f#quot;can't resolve buffer '{id}'#quot;)\n"] + block5["id = data[#quot;id#quot;]\nif id in self._buffers: + buffer = self._buffers[id] + else: + self.error(f#quot;can't resolve buffer '{id}'#quot;)\n"] + block6["elif isinstance(data, Buffer): + buffer = data + else: + id = data[#quot;id#quot;] + + if id in self._buffers: + buffer = self._buffers[id] + else: + self.error(f#quot;can't resolve buffer '{id}'#quot;)\n"] + block7["data = obj[#quot;data#quot;]\nif isinstance(data, str): + return base64.b64decode(data) + elif isinstance(data, Buffer): + buffer = data + else: + id = data[#quot;id#quot;] + + if id in self._buffers: + buffer = self._buffers[id] + else: + self.error(f#quot;can't resolve buffer '{id}'#quot;)\n"] + + start --> block7 + block7 -- "isinstance(data, str)" --> block1 + block7 -- "else" --> block6 + block6 -- "isinstance(data, Buffer)" --> block2 + block6 -- "else" --> block5 + block5 -- "id in self._buffers" --> block3 + block5 -- "else" --> block4 + block4 --> block0 + block3 --> block0 + block2 --> block0 + block1 --> return + block0 --> return +``` + + diff --git a/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__match.py.md.snap b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__match.py.md.snap new file mode 100644 index 0000000000000..d8a6ddb59bf5d --- /dev/null +++ b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__match.py.md.snap @@ -0,0 +1,776 @@ +--- +source: crates/ruff/src/rules/ruff/rules/unreachable.rs +description: "This is a Mermaid graph. You can use https://mermaid.live to visualize it as a diagram." +--- +## Function 0 +### Source +```python +def func(status): + match status: + case _: + return 0 + return "unreachable" +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return #quot;unreachable#quot;\n"] + block1["return 0\n"] + block2["match status: + case _: + return 0\n"] + + start --> block2 + block2 --> block1 + block1 --> return + block0 --> return +``` + +## Function 1 +### Source +```python +def func(status): + match status: + case 1: + return 1 + return 0 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 0\n"] + block1["return 1\n"] + block2["match status: + case 1: + return 1\n"] + + start --> block2 + block2 -- "case 1" --> block1 + block2 -- "else" --> block0 + block1 --> return + block0 --> return +``` + +## Function 2 +### Source +```python +def func(status): + match status: + case 1: + return 1 + case _: + return 0 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 0\n"] + block1["match status: + case 1: + return 1 + case _: + return 0\n"] + block2["return 1\n"] + block3["match status: + case 1: + return 1 + case _: + return 0\n"] + + start --> block3 + block3 -- "case 1" --> block2 + block3 -- "else" --> block1 + block2 --> return + block1 --> block0 + block0 --> return +``` + +## Function 3 +### Source +```python +def func(status): + match status: + case 1 | 2 | 3: + return 5 + return 6 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 6\n"] + block1["return 5\n"] + block2["match status: + case 1 | 2 | 3: + return 5\n"] + + start --> block2 + block2 -- "case 1 | 2 | 3" --> block1 + block2 -- "else" --> block0 + block1 --> return + block0 --> return +``` + +## Function 4 +### Source +```python +def func(status): + match status: + case 1 | 2 | 3: + return 5 + case _: + return 10 + return 0 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 0\n"] + block1["return 10\n"] + block2["match status: + case 1 | 2 | 3: + return 5 + case _: + return 10\n"] + block3["return 5\n"] + block4["match status: + case 1 | 2 | 3: + return 5 + case _: + return 10\n"] + + start --> block4 + block4 -- "case 1 | 2 | 3" --> block3 + block4 -- "else" --> block2 + block3 --> return + block2 --> block1 + block1 --> return + block0 --> return +``` + +## Function 5 +### Source +```python +def func(status): + match status: + case 0: + return 0 + case 1: + return 1 + case 1: + return "1 again" + case _: + return 3 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 3\n"] + block1["match status: + case 0: + return 0 + case 1: + return 1 + case 1: + return #quot;1 again#quot; + case _: + return 3\n"] + block2["return #quot;1 again#quot;\n"] + block3["match status: + case 0: + return 0 + case 1: + return 1 + case 1: + return #quot;1 again#quot; + case _: + return 3\n"] + block4["return 1\n"] + block5["match status: + case 0: + return 0 + case 1: + return 1 + case 1: + return #quot;1 again#quot; + case _: + return 3\n"] + block6["return 0\n"] + block7["match status: + case 0: + return 0 + case 1: + return 1 + case 1: + return #quot;1 again#quot; + case _: + return 3\n"] + + start --> block7 + block7 -- "case 0" --> block6 + block7 -- "else" --> block5 + block6 --> return + block5 -- "case 1" --> block4 + block5 -- "else" --> block3 + block4 --> return + block3 -- "case 1" --> block2 + block3 -- "else" --> block1 + block2 --> return + block1 --> block0 + block0 --> return +``` + +## Function 6 +### Source +```python +def func(status): + i = 0 + match status, i: + case _, _: + return 0 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["return 0\n"] + block2["match status, i: + case _, _: + return 0\n"] + block3["i = 0\n"] + + start --> block3 + block3 --> block2 + block2 -- "case _, _" --> block1 + block2 -- "else" --> block0 + block1 --> return + block0 --> return +``` + +## Function 7 +### Source +```python +def func(status): + i = 0 + match status, i: + case _, 0: + return 0 + case _, 2: + return 0 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["return 0\n"] + block2["match status, i: + case _, 0: + return 0 + case _, 2: + return 0\n"] + block3["return 0\n"] + block4["match status, i: + case _, 0: + return 0 + case _, 2: + return 0\n"] + block5["i = 0\n"] + + start --> block5 + block5 --> block4 + block4 -- "case _, 0" --> block3 + block4 -- "else" --> block2 + block3 --> return + block2 -- "case _, 2" --> block1 + block2 -- "else" --> block0 + block1 --> return + block0 --> return +``` + +## Function 8 +### Source +```python +def func(point): + match point: + case (0, 0): + print("Origin") + case _: + raise ValueError("oops") +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["raise ValueError(#quot;oops#quot;)\n"] + block2["match point: + case (0, 0): + print(#quot;Origin#quot;) + case _: + raise ValueError(#quot;oops#quot;)\n"] + block3["print(#quot;Origin#quot;)\n"] + block4["match point: + case (0, 0): + print(#quot;Origin#quot;) + case _: + raise ValueError(#quot;oops#quot;)\n"] + + start --> block4 + block4 -- "case (0, 0)" --> block3 + block4 -- "else" --> block2 + block3 --> block0 + block2 --> block1 + block1 --> return + block0 --> return +``` + +## Function 9 +### Source +```python +def func(point): + match point: + case (0, 0): + print("Origin") + case (0, y): + print(f"Y={y}") + case (x, 0): + print(f"X={x}") + case (x, y): + print(f"X={x}, Y={y}") + case _: + raise ValueError("Not a point") +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["raise ValueError(#quot;Not a point#quot;)\n"] + block2["match point: + case (0, 0): + print(#quot;Origin#quot;) + case (0, y): + print(f#quot;Y={y}#quot;) + case (x, 0): + print(f#quot;X={x}#quot;) + case (x, y): + print(f#quot;X={x}, Y={y}#quot;) + case _: + raise ValueError(#quot;Not a point#quot;)\n"] + block3["print(f#quot;X={x}, Y={y}#quot;)\n"] + block4["match point: + case (0, 0): + print(#quot;Origin#quot;) + case (0, y): + print(f#quot;Y={y}#quot;) + case (x, 0): + print(f#quot;X={x}#quot;) + case (x, y): + print(f#quot;X={x}, Y={y}#quot;) + case _: + raise ValueError(#quot;Not a point#quot;)\n"] + block5["print(f#quot;X={x}#quot;)\n"] + block6["match point: + case (0, 0): + print(#quot;Origin#quot;) + case (0, y): + print(f#quot;Y={y}#quot;) + case (x, 0): + print(f#quot;X={x}#quot;) + case (x, y): + print(f#quot;X={x}, Y={y}#quot;) + case _: + raise ValueError(#quot;Not a point#quot;)\n"] + block7["print(f#quot;Y={y}#quot;)\n"] + block8["match point: + case (0, 0): + print(#quot;Origin#quot;) + case (0, y): + print(f#quot;Y={y}#quot;) + case (x, 0): + print(f#quot;X={x}#quot;) + case (x, y): + print(f#quot;X={x}, Y={y}#quot;) + case _: + raise ValueError(#quot;Not a point#quot;)\n"] + block9["print(#quot;Origin#quot;)\n"] + block10["match point: + case (0, 0): + print(#quot;Origin#quot;) + case (0, y): + print(f#quot;Y={y}#quot;) + case (x, 0): + print(f#quot;X={x}#quot;) + case (x, y): + print(f#quot;X={x}, Y={y}#quot;) + case _: + raise ValueError(#quot;Not a point#quot;)\n"] + + start --> block10 + block10 -- "case (0, 0)" --> block9 + block10 -- "else" --> block8 + block9 --> block0 + block8 -- "case (0, y)" --> block7 + block8 -- "else" --> block6 + block7 --> block0 + block6 -- "case (x, 0)" --> block5 + block6 -- "else" --> block4 + block5 --> block0 + block4 -- "case (x, y)" --> block3 + block4 -- "else" --> block2 + block3 --> block0 + block2 --> block1 + block1 --> return + block0 --> return +``` + +## Function 10 +### Source +```python +def where_is(point): + class Point: + x: int + y: int + + match point: + case Point(x=0, y=0): + print("Origin") + case Point(x=0, y=y): + print(f"Y={y}") + case Point(x=x, y=0): + print(f"X={x}") + case Point(): + print("Somewhere else") + case _: + print("Not a point") +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["print(#quot;Not a point#quot;)\n"] + block2["match point: + case Point(x=0, y=0): + print(#quot;Origin#quot;) + case Point(x=0, y=y): + print(f#quot;Y={y}#quot;) + case Point(x=x, y=0): + print(f#quot;X={x}#quot;) + case Point(): + print(#quot;Somewhere else#quot;) + case _: + print(#quot;Not a point#quot;)\n"] + block3["print(#quot;Somewhere else#quot;)\n"] + block4["match point: + case Point(x=0, y=0): + print(#quot;Origin#quot;) + case Point(x=0, y=y): + print(f#quot;Y={y}#quot;) + case Point(x=x, y=0): + print(f#quot;X={x}#quot;) + case Point(): + print(#quot;Somewhere else#quot;) + case _: + print(#quot;Not a point#quot;)\n"] + block5["print(f#quot;X={x}#quot;)\n"] + block6["match point: + case Point(x=0, y=0): + print(#quot;Origin#quot;) + case Point(x=0, y=y): + print(f#quot;Y={y}#quot;) + case Point(x=x, y=0): + print(f#quot;X={x}#quot;) + case Point(): + print(#quot;Somewhere else#quot;) + case _: + print(#quot;Not a point#quot;)\n"] + block7["print(f#quot;Y={y}#quot;)\n"] + block8["match point: + case Point(x=0, y=0): + print(#quot;Origin#quot;) + case Point(x=0, y=y): + print(f#quot;Y={y}#quot;) + case Point(x=x, y=0): + print(f#quot;X={x}#quot;) + case Point(): + print(#quot;Somewhere else#quot;) + case _: + print(#quot;Not a point#quot;)\n"] + block9["print(#quot;Origin#quot;)\n"] + block10["match point: + case Point(x=0, y=0): + print(#quot;Origin#quot;) + case Point(x=0, y=y): + print(f#quot;Y={y}#quot;) + case Point(x=x, y=0): + print(f#quot;X={x}#quot;) + case Point(): + print(#quot;Somewhere else#quot;) + case _: + print(#quot;Not a point#quot;)\n"] + block11["class Point: + x: int + y: int\n"] + + start --> block11 + block11 --> block10 + block10 -- "case Point(x=0, y=0)" --> block9 + block10 -- "else" --> block8 + block9 --> block0 + block8 -- "case Point(x=0, y=y)" --> block7 + block8 -- "else" --> block6 + block7 --> block0 + block6 -- "case Point(x=x, y=0)" --> block5 + block6 -- "else" --> block4 + block5 --> block0 + block4 -- "case Point()" --> block3 + block4 -- "else" --> block2 + block3 --> block0 + block2 --> block1 + block1 --> block0 + block0 --> return +``` + +## Function 11 +### Source +```python +def func(points): + match points: + case []: + print("No points") + case [Point(0, 0)]: + print("The origin") + case [Point(x, y)]: + print(f"Single point {x}, {y}") + case [Point(0, y1), Point(0, y2)]: + print(f"Two on the Y axis at {y1}, {y2}") + case _: + print("Something else") +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["print(#quot;Something else#quot;)\n"] + block2["match points: + case []: + print(#quot;No points#quot;) + case [Point(0, 0)]: + print(#quot;The origin#quot;) + case [Point(x, y)]: + print(f#quot;Single point {x}, {y}#quot;) + case [Point(0, y1), Point(0, y2)]: + print(f#quot;Two on the Y axis at {y1}, {y2}#quot;) + case _: + print(#quot;Something else#quot;)\n"] + block3["print(f#quot;Two on the Y axis at {y1}, {y2}#quot;)\n"] + block4["match points: + case []: + print(#quot;No points#quot;) + case [Point(0, 0)]: + print(#quot;The origin#quot;) + case [Point(x, y)]: + print(f#quot;Single point {x}, {y}#quot;) + case [Point(0, y1), Point(0, y2)]: + print(f#quot;Two on the Y axis at {y1}, {y2}#quot;) + case _: + print(#quot;Something else#quot;)\n"] + block5["print(f#quot;Single point {x}, {y}#quot;)\n"] + block6["match points: + case []: + print(#quot;No points#quot;) + case [Point(0, 0)]: + print(#quot;The origin#quot;) + case [Point(x, y)]: + print(f#quot;Single point {x}, {y}#quot;) + case [Point(0, y1), Point(0, y2)]: + print(f#quot;Two on the Y axis at {y1}, {y2}#quot;) + case _: + print(#quot;Something else#quot;)\n"] + block7["print(#quot;The origin#quot;)\n"] + block8["match points: + case []: + print(#quot;No points#quot;) + case [Point(0, 0)]: + print(#quot;The origin#quot;) + case [Point(x, y)]: + print(f#quot;Single point {x}, {y}#quot;) + case [Point(0, y1), Point(0, y2)]: + print(f#quot;Two on the Y axis at {y1}, {y2}#quot;) + case _: + print(#quot;Something else#quot;)\n"] + block9["print(#quot;No points#quot;)\n"] + block10["match points: + case []: + print(#quot;No points#quot;) + case [Point(0, 0)]: + print(#quot;The origin#quot;) + case [Point(x, y)]: + print(f#quot;Single point {x}, {y}#quot;) + case [Point(0, y1), Point(0, y2)]: + print(f#quot;Two on the Y axis at {y1}, {y2}#quot;) + case _: + print(#quot;Something else#quot;)\n"] + + start --> block10 + block10 -- "case []" --> block9 + block10 -- "else" --> block8 + block9 --> block0 + block8 -- "case [Point(0, 0)]" --> block7 + block8 -- "else" --> block6 + block7 --> block0 + block6 -- "case [Point(x, y)]" --> block5 + block6 -- "else" --> block4 + block5 --> block0 + block4 -- "case [Point(0, y1), Point(0, y2)]" --> block3 + block4 -- "else" --> block2 + block3 --> block0 + block2 --> block1 + block1 --> block0 + block0 --> return +``` + +## Function 12 +### Source +```python +def func(point): + match point: + case Point(x, y) if x == y: + print(f"Y=X at {x}") + case Point(x, y): + print(f"Not on the diagonal") +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["print(f#quot;Not on the diagonal#quot;)\n"] + block2["match point: + case Point(x, y) if x == y: + print(f#quot;Y=X at {x}#quot;) + case Point(x, y): + print(f#quot;Not on the diagonal#quot;)\n"] + block3["print(f#quot;Y=X at {x}#quot;)\n"] + block4["match point: + case Point(x, y) if x == y: + print(f#quot;Y=X at {x}#quot;) + case Point(x, y): + print(f#quot;Not on the diagonal#quot;)\n"] + + start --> block4 + block4 -- "case Point(x, y) if x == y" --> block3 + block4 -- "else" --> block2 + block3 --> block0 + block2 -- "case Point(x, y)" --> block1 + block2 -- "else" --> block0 + block1 --> block0 + block0 --> return +``` + +## Function 13 +### Source +```python +def func(): + from enum import Enum + class Color(Enum): + RED = 'red' + GREEN = 'green' + BLUE = 'blue' + + color = Color(input("Enter your choice of 'red', 'blue' or 'green': ")) + + match color: + case Color.RED: + print("I see red!") + case Color.GREEN: + print("Grass is green") + case Color.BLUE: + print("I'm feeling the blues :(") +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["print(#quot;I'm feeling the blues :(#quot;)\n"] + block2["match color: + case Color.RED: + print(#quot;I see red!#quot;) + case Color.GREEN: + print(#quot;Grass is green#quot;) + case Color.BLUE: + print(#quot;I'm feeling the blues :(#quot;)\n"] + block3["print(#quot;Grass is green#quot;)\n"] + block4["match color: + case Color.RED: + print(#quot;I see red!#quot;) + case Color.GREEN: + print(#quot;Grass is green#quot;) + case Color.BLUE: + print(#quot;I'm feeling the blues :(#quot;)\n"] + block5["print(#quot;I see red!#quot;)\n"] + block6["match color: + case Color.RED: + print(#quot;I see red!#quot;) + case Color.GREEN: + print(#quot;Grass is green#quot;) + case Color.BLUE: + print(#quot;I'm feeling the blues :(#quot;)\n"] + block7["from enum import Enum\nclass Color(Enum): + RED = 'red' + GREEN = 'green' + BLUE = 'blue'\ncolor = Color(input(#quot;Enter your choice of 'red', 'blue' or 'green': #quot;))\n"] + + start --> block7 + block7 --> block6 + block6 -- "case Color.RED" --> block5 + block6 -- "else" --> block4 + block5 --> block0 + block4 -- "case Color.GREEN" --> block3 + block4 -- "else" --> block2 + block3 --> block0 + block2 -- "case Color.BLUE" --> block1 + block2 -- "else" --> block0 + block1 --> block0 + block0 --> return +``` + + diff --git a/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__raise.py.md.snap b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__raise.py.md.snap new file mode 100644 index 0000000000000..7da998458d7ee --- /dev/null +++ b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__raise.py.md.snap @@ -0,0 +1,41 @@ +--- +source: crates/ruff/src/rules/ruff/rules/unreachable.rs +description: "This is a Mermaid graph. You can use https://mermaid.live to visualize it as a diagram." +--- +## Function 0 +### Source +```python +def func(): + raise Exception +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["raise Exception\n"] + + start --> block0 + block0 --> return +``` + +## Function 1 +### Source +```python +def func(): + raise "a glass!" +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["raise #quot;a glass!#quot;\n"] + + start --> block0 + block0 --> return +``` + + diff --git a/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__simple.py.md.snap b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__simple.py.md.snap new file mode 100644 index 0000000000000..881df6fad13c4 --- /dev/null +++ b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__simple.py.md.snap @@ -0,0 +1,136 @@ +--- +source: crates/ruff/src/rules/ruff/rules/unreachable.rs +description: "This is a Mermaid graph. You can use https://mermaid.live to visualize it as a diagram." +--- +## Function 0 +### Source +```python +def func(): + pass +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["pass\n"] + + start --> block0 + block0 --> return +``` + +## Function 1 +### Source +```python +def func(): + pass +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["pass\n"] + + start --> block0 + block0 --> return +``` + +## Function 2 +### Source +```python +def func(): + return +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return\n"] + + start --> block0 + block0 --> return +``` + +## Function 3 +### Source +```python +def func(): + return 1 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 1\n"] + + start --> block0 + block0 --> return +``` + +## Function 4 +### Source +```python +def func(): + return 1 + return "unreachable" +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return #quot;unreachable#quot;\n"] + block1["return 1\n"] + + start --> block1 + block1 --> return + block0 --> return +``` + +## Function 5 +### Source +```python +def func(): + i = 0 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["i = 0\n"] + + start --> block0 + block0 --> return +``` + +## Function 6 +### Source +```python +def func(): + i = 0 + i += 2 + return i +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["i = 0\ni += 2\nreturn i\n"] + + start --> block0 + block0 --> return +``` + + diff --git a/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__while.py.md.snap b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__while.py.md.snap new file mode 100644 index 0000000000000..aa030a03a4db5 --- /dev/null +++ b/crates/ruff/src/rules/ruff/rules/snapshots/ruff__rules__ruff__rules__unreachable__tests__while.py.md.snap @@ -0,0 +1,527 @@ +--- +source: crates/ruff/src/rules/ruff/rules/unreachable.rs +description: "This is a Mermaid graph. You can use https://mermaid.live to visualize it as a diagram." +--- +## Function 0 +### Source +```python +def func(): + while False: + return "unreachable" + return 1 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 1\n"] + block1["return #quot;unreachable#quot;\n"] + block2["while False: + return #quot;unreachable#quot;\n"] + + start --> block2 + block2 -- "False" --> block1 + block2 -- "else" --> block0 + block1 --> return + block0 --> return +``` + +## Function 1 +### Source +```python +def func(): + while False: + return "unreachable" + else: + return 1 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return #quot;unreachable#quot;\n"] + block1["return 1\n"] + block2["while False: + return #quot;unreachable#quot; + else: + return 1\n"] + + start --> block2 + block2 -- "False" --> block0 + block2 -- "else" --> block1 + block1 --> return + block0 --> return +``` + +## Function 2 +### Source +```python +def func(): + while False: + return "unreachable" + else: + return 1 + return "also unreachable" +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return #quot;also unreachable#quot;\n"] + block1["return #quot;unreachable#quot;\n"] + block2["return 1\n"] + block3["while False: + return #quot;unreachable#quot; + else: + return 1\n"] + + start --> block3 + block3 -- "False" --> block1 + block3 -- "else" --> block2 + block2 --> return + block1 --> return + block0 --> return +``` + +## Function 3 +### Source +```python +def func(): + while True: + return 1 + return "unreachable" +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return #quot;unreachable#quot;\n"] + block1["return 1\n"] + block2["while True: + return 1\n"] + + start --> block2 + block2 -- "True" --> block1 + block2 -- "else" --> block0 + block1 --> return + block0 --> return +``` + +## Function 4 +### Source +```python +def func(): + while True: + return 1 + else: + return "unreachable" +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 1\n"] + block1["return #quot;unreachable#quot;\n"] + block2["while True: + return 1 + else: + return #quot;unreachable#quot;\n"] + + start --> block2 + block2 -- "True" --> block0 + block2 -- "else" --> block1 + block1 --> return + block0 --> return +``` + +## Function 5 +### Source +```python +def func(): + while True: + return 1 + else: + return "unreachable" + return "also unreachable" +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return #quot;also unreachable#quot;\n"] + block1["return 1\n"] + block2["return #quot;unreachable#quot;\n"] + block3["while True: + return 1 + else: + return #quot;unreachable#quot;\n"] + + start --> block3 + block3 -- "True" --> block1 + block3 -- "else" --> block2 + block2 --> return + block1 --> return + block0 --> return +``` + +## Function 6 +### Source +```python +def func(): + i = 0 + while False: + i += 1 + return i +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return i\n"] + block1["i += 1\n"] + block2["i = 0\nwhile False: + i += 1\n"] + + start --> block2 + block2 -- "False" --> block1 + block2 -- "else" --> block0 + block1 --> block2 + block0 --> return +``` + +## Function 7 +### Source +```python +def func(): + i = 0 + while True: + i += 1 + return i +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return i\n"] + block1["i += 1\n"] + block2["i = 0\nwhile True: + i += 1\n"] + + start --> block2 + block2 -- "True" --> block1 + block2 -- "else" --> block0 + block1 --> block2 + block0 --> return +``` + +## Function 8 +### Source +```python +def func(): + while True: + pass + return 1 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 1\n"] + block1["pass\n"] + block2["while True: + pass\n"] + + start --> block2 + block2 -- "True" --> block1 + block2 -- "else" --> block0 + block1 --> block2 + block0 --> return +``` + +## Function 9 +### Source +```python +def func(): + i = 0 + while True: + if True: + print("ok") + i += 1 + return i +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return i\n"] + block1["i += 1\n"] + block2["print(#quot;ok#quot;)\n"] + block3["if True: + print(#quot;ok#quot;)\n"] + block4["i = 0\nwhile True: + if True: + print(#quot;ok#quot;) + i += 1\n"] + + start --> block4 + block4 -- "True" --> block3 + block4 -- "else" --> block0 + block3 -- "True" --> block2 + block3 -- "else" --> block1 + block2 --> block1 + block1 --> block4 + block0 --> return +``` + +## Function 10 +### Source +```python +def func(): + i = 0 + while True: + if False: + print("ok") + i += 1 + return i +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return i\n"] + block1["i += 1\n"] + block2["print(#quot;ok#quot;)\n"] + block3["if False: + print(#quot;ok#quot;)\n"] + block4["i = 0\nwhile True: + if False: + print(#quot;ok#quot;) + i += 1\n"] + + start --> block4 + block4 -- "True" --> block3 + block4 -- "else" --> block0 + block3 -- "False" --> block2 + block3 -- "else" --> block1 + block2 --> block1 + block1 --> block4 + block0 --> return +``` + +## Function 11 +### Source +```python +def func(): + while True: + if True: + return 1 + return 0 +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0["return 0\n"] + block1["return 1\n"] + block2["if True: + return 1\n"] + block3["while True: + if True: + return 1\n"] + + start --> block3 + block3 -- "True" --> block2 + block3 -- "else" --> block0 + block2 -- "True" --> block1 + block2 -- "else" --> block3 + block1 --> return + block0 --> return +``` + +## Function 12 +### Source +```python +def func(): + while True: + continue +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["continue\n"] + block2["while True: + continue\n"] + + start --> block2 + block2 -- "True" --> block1 + block2 -- "else" --> block0 + block1 --> block2 + block0 --> return +``` + +## Function 13 +### Source +```python +def func(): + while False: + continue +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["continue\n"] + block2["while False: + continue\n"] + + start --> block2 + block2 -- "False" --> block1 + block2 -- "else" --> block0 + block1 --> block2 + block0 --> return +``` + +## Function 14 +### Source +```python +def func(): + while True: + break +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["break\n"] + block2["while True: + break\n"] + + start --> block2 + block2 -- "True" --> block1 + block2 -- "else" --> block0 + block1 --> block0 + block0 --> return +``` + +## Function 15 +### Source +```python +def func(): + while False: + break +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["break\n"] + block2["while False: + break\n"] + + start --> block2 + block2 -- "False" --> block1 + block2 -- "else" --> block0 + block1 --> block0 + block0 --> return +``` + +## Function 16 +### Source +```python +def func(): + while True: + if True: + continue +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["continue\n"] + block2["if True: + continue\n"] + block3["while True: + if True: + continue\n"] + + start --> block3 + block3 -- "True" --> block2 + block3 -- "else" --> block0 + block2 -- "True" --> block1 + block2 -- "else" --> block3 + block1 --> block3 + block0 --> return +``` + +## Function 17 +### Source +```python +def func(): + while True: + if True: + break +``` + +### Control Flow Graph +```mermaid +flowchart TD + start(("Start")) + return(("End")) + block0[["`*(empty)*`"]] + block1["break\n"] + block2["if True: + break\n"] + block3["while True: + if True: + break\n"] + + start --> block3 + block3 -- "True" --> block2 + block3 -- "else" --> block0 + block2 -- "True" --> block1 + block2 -- "else" --> block3 + block1 --> block0 + block0 --> return +``` + + diff --git a/crates/ruff/src/rules/ruff/rules/unreachable.rs b/crates/ruff/src/rules/ruff/rules/unreachable.rs new file mode 100644 index 0000000000000..8aede6a61372f --- /dev/null +++ b/crates/ruff/src/rules/ruff/rules/unreachable.rs @@ -0,0 +1,1101 @@ +use std::{fmt, iter, usize}; + +use log::error; +use rustpython_parser::ast::{ + Expr, Identifier, MatchCase, Pattern, PatternMatchAs, Ranged, Stmt, StmtAsyncFor, + StmtAsyncWith, StmtFor, StmtMatch, StmtReturn, StmtTry, StmtTryStar, StmtWhile, StmtWith, +}; +use rustpython_parser::text_size::{TextRange, TextSize}; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_index::{IndexSlice, IndexVec}; +use ruff_macros::{derive_message_formats, newtype_index, violation}; + +/// ## What it does +/// Checks for unreachable code. +/// +/// ## Why is this bad? +/// Unreachable code can be a maintenance burden without ever being used. +/// +/// ## Example +/// ```python +/// def function(): +/// if False: +/// return "unreachable" +/// return "reachable" +/// ``` +/// +/// Use instead: +/// ```python +/// def function(): +/// return "reachable" +/// ``` +#[violation] +pub struct UnreachableCode { + name: String, +} + +impl Violation for UnreachableCode { + #[derive_message_formats] + fn message(&self) -> String { + let UnreachableCode { name } = self; + format!("Unreachable code in {name}") + } +} + +pub(crate) fn in_function(name: &Identifier, body: &[Stmt]) -> Vec { + // Create basic code blocks from the body. + let basic_blocks = BasicBlocks::from(body); + + // Basic on the code blocks we can (more) easily follow what statements are + // and aren't reached, we'll mark them as such in `reached_map`. + let mut reached_map = Bitmap::with_capacity(basic_blocks.len()); + + if let Some(start_index) = basic_blocks.start_index() { + mark_reached(&mut reached_map, &basic_blocks.blocks, start_index); + } + + // For each unreached code block create a diagnostic. + reached_map + .unset() + .filter_map(|idx| { + let block = &basic_blocks.blocks[idx]; + if block.is_sentinel() { + return None; + } + + // TODO: add more information to the diagnostic. Include the entire + // code block, not just the first line. Maybe something to indicate + // the code flow and where it prevents this block from being reached + // for example. + let Some(stmt) = block.stmts.first() else { + // This should never happen. + error!("Got an unexpected empty code block"); + return None; + }; + Some(Diagnostic::new( + UnreachableCode { + name: name.as_str().to_owned(), + }, + stmt.range(), + )) + }) + .collect() +} + +/// Simple bitmap. +#[derive(Debug)] +struct Bitmap { + bits: Box<[usize]>, + capacity: usize, +} + +impl Bitmap { + /// Create a new `Bitmap` with `capacity` capacity. + fn with_capacity(capacity: usize) -> Bitmap { + let mut size = capacity / usize::BITS as usize; + if (capacity % usize::BITS as usize) != 0 { + size += 1; + } + Bitmap { + bits: vec![0; size].into_boxed_slice(), + capacity, + } + } + + /// Set bit at index `idx` to true. + /// + /// Returns a boolean indicating if the bit was already set. + fn set(&mut self, idx: BlockIndex) -> bool { + let bits_index = (idx.as_u32() / usize::BITS) as usize; + let shift = idx.as_u32() % usize::BITS; + if (self.bits[bits_index] & (1 << shift)) == 0 { + self.bits[bits_index] |= 1 << shift; + false + } else { + true + } + } + + /// Returns an iterator of all unset indices. + fn unset(&self) -> impl Iterator + '_ { + let mut index = 0; + let mut shift = 0; + let last_max_shift = self.capacity % usize::BITS as usize; + iter::from_fn(move || loop { + if shift >= usize::BITS as usize { + shift = 0; + index += 1; + } + if self.bits.len() <= index || (index >= self.bits.len() - 1 && shift >= last_max_shift) + { + return None; + } + + let is_set = (self.bits[index] & (1 << shift)) != 0; + shift += 1; + if !is_set { + return Some(BlockIndex::from_usize( + (index * usize::BITS as usize) + shift - 1, + )); + } + }) + } +} + +/// Set bits in `reached_map` for all blocks that are reached in `blocks` +/// starting with block at index `idx`. +fn mark_reached( + reached_map: &mut Bitmap, + blocks: &IndexSlice>, + start_index: BlockIndex, +) { + let mut idx = start_index; + + loop { + let block = &blocks[idx]; + if reached_map.set(idx) { + return; // Block already visited, no needed to do it again. + } + + match &block.next { + NextBlock::Always(next) => idx = *next, + NextBlock::If { + condition, + next, + orelse, + } => { + match taken(condition) { + Some(true) => idx = *next, // Always taken. + Some(false) => idx = *orelse, // Never taken. + None => { + // Don't know, both branches might be taken. + idx = *next; + mark_reached(reached_map, blocks, *orelse); + } + } + } + NextBlock::Terminate => return, + } + } +} + +/// Determines if `condition` is taken. +/// Returns `Some(true)` if the condition is always true, e.g. `if True`, same +/// with `Some(false)` if it's never taken. If it can't be determined it returns +/// `None`, e.g. `If i == 100`. +fn taken(condition: &Condition) -> Option { + // TODO: add more cases to this where we can determine a condition + // statically. For now we only consider constant booleans. + match condition { + Condition::Test(expr) => match expr { + Expr::Constant(constant) => constant.value.as_bool().copied(), + _ => None, + }, + Condition::Iterator(_) => None, + Condition::Match { .. } => None, + } +} + +/// Index into [`BasicBlocks::blocks`]. +#[newtype_index] +#[derive(PartialOrd, Ord)] +struct BlockIndex; + +/// Collection of basic block. +#[derive(Debug, PartialEq)] +struct BasicBlocks<'stmt> { + /// # Notes + /// + /// The order of these block is unspecified. However it's guaranteed that + /// the last block is the first statement in the function and the first + /// block is the last statement. The block are more or less in reverse + /// order, but it gets fussy around control flow statements (e.g. `while` + /// statements). + /// + /// For loop blocks, and similar recurring control flows, the end of the + /// body will point to the loop block again (to create the loop). However an + /// oddity here is that this block might contain statements before the loop + /// itself which, of course, won't be executed again. + /// + /// For example: + /// ```python + /// i = 0 # block 0 + /// while True: # + /// continue # block 1 + /// ``` + /// Will create a connection between block 1 (loop body) and block 0, which + /// includes the `i = 0` statement. + /// + /// To keep `NextBlock` simple(r) `NextBlock::If`'s `next` and `orelse` + /// fields only use `BlockIndex`, which means that they can't terminate + /// themselves. To support this we insert *empty*/fake blocks before the end + /// of the function that we can link to. + /// + /// Finally `BasicBlock` can also be a sentinel node, see the associated + /// constants of [`BasicBlock`]. + blocks: IndexVec>, +} + +impl BasicBlocks<'_> { + fn len(&self) -> usize { + self.blocks.len() + } + + fn start_index(&self) -> Option { + self.blocks.indices().last() + } +} + +impl<'stmt> From<&'stmt [Stmt]> for BasicBlocks<'stmt> { + /// # Notes + /// + /// This assumes that `stmts` is a function body. + fn from(stmts: &'stmt [Stmt]) -> BasicBlocks<'stmt> { + let mut blocks = BasicBlocksBuilder::with_capacity(stmts.len()); + + blocks.create_blocks(stmts, None); + + blocks.finish() + } +} + +/// Basic code block, sequence of statements unconditionally executed +/// "together". +#[derive(Debug, PartialEq)] +struct BasicBlock<'stmt> { + stmts: &'stmt [Stmt], + next: NextBlock<'stmt>, +} + +/// Edge between basic blocks (in the control-flow graph). +#[derive(Debug, PartialEq)] +enum NextBlock<'stmt> { + /// Always continue with a block. + Always(BlockIndex), + /// Condition jump. + If { + /// Condition that needs to be evaluated to jump to the `next` or + /// `orelse` block. + condition: Condition<'stmt>, + /// Next block if `condition` is true. + next: BlockIndex, + /// Next block if `condition` is false. + orelse: BlockIndex, + }, + /// The end. + Terminate, +} + +/// Condition used to determine to take the `next` or `orelse` branch in +/// [`NextBlock::If`]. +#[derive(Clone, Debug, PartialEq)] +enum Condition<'stmt> { + /// Conditional statement, this should evaluate to a boolean, for e.g. `if` + /// or `while`. + Test(&'stmt Expr), + /// Iterator for `for` statements, e.g. for `i in range(10)` this will be + /// `range(10)`. + Iterator(&'stmt Expr), + Match { + /// `match $subject`. + subject: &'stmt Expr, + /// `case $case`, include pattern, guard, etc. + case: &'stmt MatchCase, + }, +} + +impl<'stmt> Ranged for Condition<'stmt> { + fn range(&self) -> TextRange { + match self { + Condition::Test(expr) | Condition::Iterator(expr) => expr.range(), + // The case of the match statement, without the body. + Condition::Match { subject: _, case } => TextRange::new( + case.start(), + case.guard + .as_ref() + .map_or(case.pattern.end(), |guard| guard.end()), + ), + } + } +} + +impl<'stmt> BasicBlock<'stmt> { + /// A sentinel block indicating an empty termination block. + const EMPTY: BasicBlock<'static> = BasicBlock { + stmts: &[], + next: NextBlock::Terminate, + }; + + /// A sentinel block indicating an exception was raised. + const EXCEPTION: BasicBlock<'static> = BasicBlock { + stmts: &[Stmt::Return(StmtReturn { + range: TextRange::new(TextSize::new(0), TextSize::new(0)), + value: None, + })], + next: NextBlock::Terminate, + }; + + /// Return true if the block is a sentinel or fake block. + fn is_sentinel(&self) -> bool { + self.is_empty() || self.is_exception() + } + + /// Returns an empty block that terminates. + fn is_empty(&self) -> bool { + matches!(self.next, NextBlock::Terminate) && self.stmts.is_empty() + } + + /// Returns true if `self` an [`BasicBlock::EXCEPTION`]. + fn is_exception(&self) -> bool { + matches!(self.next, NextBlock::Terminate) && BasicBlock::EXCEPTION.stmts == self.stmts + } +} + +/// Handle a loop block, such as a `while`, `for` or `async for` statement. +fn loop_block<'stmt>( + blocks: &mut BasicBlocksBuilder<'stmt>, + condition: Condition<'stmt>, + body: &'stmt [Stmt], + orelse: &'stmt [Stmt], + after: Option, +) -> NextBlock<'stmt> { + let after_block = blocks.maybe_next_block_index(after, || orelse.is_empty()); + // NOTE: a while loop's body must not be empty, so we can safely + // create at least one block from it. + let last_statement_index = blocks.append_blocks(body, after); + let last_orelse_statement = blocks.append_blocks_if_not_empty(orelse, after_block); + // `create_blocks` always continues to the next block by + // default. However in a while loop we want to continue with the + // while block (we're about to create) to create the loop. + // NOTE: `blocks.len()` is an invalid index at time of creation + // as it points to the block which we're about to create. + blocks.change_next_block( + last_statement_index, + after_block, + blocks.blocks.next_index(), + |block| { + // For `break` statements we don't want to continue with the + // loop, but instead with the statement after the loop (i.e. + // not change anything). + !block.stmts.last().map_or(false, Stmt::is_break_stmt) + }, + ); + NextBlock::If { + condition, + next: last_statement_index, + orelse: last_orelse_statement, + } +} + +/// Handle a single match case. +/// +/// `next_after_block` is the block *after* the entire match statement that is +/// taken after this case is taken. +/// `orelse_after_block` is the next match case (or the block after the match +/// statement if this is the last case). +fn match_case<'stmt>( + blocks: &mut BasicBlocksBuilder<'stmt>, + match_stmt: &'stmt Stmt, + subject: &'stmt Expr, + case: &'stmt MatchCase, + next_after_block: BlockIndex, + orelse_after_block: BlockIndex, +) -> BasicBlock<'stmt> { + // FIXME: this is not ideal, we want to only use the `case` statement here, + // but that is type `MatchCase`, not `Stmt`. For now we'll point to the + // entire match statement. + let stmts = std::slice::from_ref(match_stmt); + let next_block_index = if case.body.is_empty() { + next_after_block + } else { + let from = blocks.last_index(); + let last_statement_index = blocks.append_blocks(&case.body, Some(next_after_block)); + if let Some(from) = from { + blocks.change_next_block(last_statement_index, from, next_after_block, |_| true); + } + last_statement_index + }; + // TODO: handle named arguments, e.g. + // ```python + // match $subject: + // case $binding: + // print($binding) + // ``` + // These should also return `NextBlock::Always`. + let next = if is_wildcard(case) { + // Wildcard case is always taken. + NextBlock::Always(next_block_index) + } else { + NextBlock::If { + condition: Condition::Match { subject, case }, + next: next_block_index, + orelse: orelse_after_block, + } + }; + BasicBlock { stmts, next } +} + +/// Returns true if `pattern` is a wildcard (`_`) pattern. +fn is_wildcard(pattern: &MatchCase) -> bool { + pattern.guard.is_none() + && matches!(&pattern.pattern, Pattern::MatchAs(PatternMatchAs { pattern, name, .. }) if pattern.is_none() && name.is_none()) +} + +#[derive(Debug, Default)] +struct BasicBlocksBuilder<'stmt> { + blocks: IndexVec>, +} + +impl<'stmt> BasicBlocksBuilder<'stmt> { + fn with_capacity(capacity: usize) -> Self { + Self { + blocks: IndexVec::with_capacity(capacity), + } + } + + /// Creates basic blocks from `stmts` and appends them to `blocks`. + fn create_blocks( + &mut self, + stmts: &'stmt [Stmt], + mut after: Option, + ) -> Option { + // We process the statements in reverse so that we can always point to the + // next block (as that should always be processed). + let mut stmts_iter = stmts.iter().enumerate().rev().peekable(); + while let Some((i, stmt)) = stmts_iter.next() { + let next = match stmt { + // Statements that continue to the next statement after execution. + Stmt::FunctionDef(_) + | Stmt::AsyncFunctionDef(_) + | Stmt::Import(_) + | Stmt::ImportFrom(_) + | Stmt::ClassDef(_) + | Stmt::Global(_) + | Stmt::Nonlocal(_) + | Stmt::Delete(_) + | Stmt::Assign(_) + | Stmt::AugAssign(_) + | Stmt::AnnAssign(_) + | Stmt::Break(_) + | Stmt::Pass(_) => self.unconditional_next_block(after), + Stmt::Continue(_) => { + // NOTE: the next branch gets fixed up in `change_next_block`. + self.unconditional_next_block(after) + } + // Statements that (can) divert the control flow. + Stmt::If(stmt) => { + let next_after_block = + self.maybe_next_block_index(after, || needs_next_block(&stmt.body)); + let orelse_after_block = + self.maybe_next_block_index(after, || needs_next_block(&stmt.orelse)); + let next = self.append_blocks_if_not_empty(&stmt.body, next_after_block); + let orelse = self.append_blocks_if_not_empty(&stmt.orelse, orelse_after_block); + NextBlock::If { + condition: Condition::Test(&stmt.test), + next, + orelse, + } + } + Stmt::While(StmtWhile { + test: condition, + body, + orelse, + .. + }) => loop_block(self, Condition::Test(condition), body, orelse, after), + Stmt::For(StmtFor { + iter: condition, + body, + orelse, + .. + }) + | Stmt::AsyncFor(StmtAsyncFor { + iter: condition, + body, + orelse, + .. + }) => loop_block(self, Condition::Iterator(condition), body, orelse, after), + Stmt::Try(StmtTry { + body, + handlers, + orelse, + finalbody, + .. + }) + | Stmt::TryStar(StmtTryStar { + body, + handlers, + orelse, + finalbody, + .. + }) => { + // TODO: handle `try` statements. The `try` control flow is very + // complex, what blocks are and aren't taken and from which + // block the control flow is actually returns is **very** + // specific to the contents of the block. Read + // + // very carefully. + // For now we'll skip over it. + let _ = (body, handlers, orelse, finalbody); // Silence unused code warnings. + self.unconditional_next_block(after) + } + Stmt::With(StmtWith { + items, + body, + type_comment, + .. + }) + | Stmt::AsyncWith(StmtAsyncWith { + items, + body, + type_comment, + .. + }) => { + // TODO: handle `with` statements, see + // . + // I recommend to `try` statements first as `with` can desugar + // to a `try` statement. + // For now we'll skip over it. + let _ = (items, body, type_comment); // Silence unused code warnings. + self.unconditional_next_block(after) + } + Stmt::Match(StmtMatch { subject, cases, .. }) => { + let next_after_block = self.maybe_next_block_index(after, || { + // We don't need need a next block if all cases don't need a + // next block, i.e. if no cases need a next block, and we + // have a wildcard case (to ensure one of the block is + // always taken). + // NOTE: match statement require at least one case, so we + // don't have to worry about empty `cases`. + // TODO: support exhaustive cases without a wildcard. + cases.iter().any(|case| needs_next_block(&case.body)) + || !cases.iter().any(is_wildcard) + }); + let mut orelse_after_block = next_after_block; + for case in cases.iter().rev() { + let block = match_case( + self, + stmt, + subject, + case, + next_after_block, + orelse_after_block, + ); + // For the case above this use the just added case as the + // `orelse` branch, this convert the match statement to + // (essentially) a bunch of if statements. + orelse_after_block = self.blocks.push(block); + } + // TODO: currently we don't include the lines before the match + // statement in the block, unlike what we do for other + // statements. + after = Some(orelse_after_block); + continue; + } + Stmt::Raise(_) => { + // TODO: this needs special handling within `try` and `with` + // statements. For now we just terminate the execution, it's + // possible it's continued in an `catch` or `finally` block, + // possibly outside of the function. + // Also see `Stmt::Assert` handling. + NextBlock::Terminate + } + Stmt::Assert(stmt) => { + // TODO: this needs special handling within `try` and `with` + // statements. For now we just terminate the execution if the + // assertion fails, it's possible it's continued in an `catch` + // or `finally` block, possibly outside of the function. + // Also see `Stmt::Raise` handling. + let next = self.force_next_block_index(); + let orelse = self.fake_exception_block_index(); + NextBlock::If { + condition: Condition::Test(&stmt.test), + next, + orelse, + } + } + Stmt::Expr(stmt) => { + match &*stmt.value { + Expr::BoolOp(_) + | Expr::BinOp(_) + | Expr::UnaryOp(_) + | Expr::Dict(_) + | Expr::Set(_) + | Expr::Compare(_) + | Expr::Call(_) + | Expr::FormattedValue(_) + | Expr::JoinedStr(_) + | Expr::Constant(_) + | Expr::Attribute(_) + | Expr::Subscript(_) + | Expr::Starred(_) + | Expr::Name(_) + | Expr::List(_) + | Expr::Tuple(_) + | Expr::Slice(_) => self.unconditional_next_block(after), + // TODO: handle these expressions. + Expr::NamedExpr(_) + | Expr::Lambda(_) + | Expr::IfExp(_) + | Expr::ListComp(_) + | Expr::SetComp(_) + | Expr::DictComp(_) + | Expr::GeneratorExp(_) + | Expr::Await(_) + | Expr::Yield(_) + | Expr::YieldFrom(_) => self.unconditional_next_block(after), + } + } + // The tough branches are done, here is an easy one. + Stmt::Return(_) => NextBlock::Terminate, + }; + + // Include any statements in the block that don't divert the control flow. + let mut start = i; + let end = i + 1; + while stmts_iter + .next_if(|(_, stmt)| !is_control_flow_stmt(stmt)) + .is_some() + { + start -= 1; + } + + let block = BasicBlock { + stmts: &stmts[start..end], + next, + }; + after = Some(self.blocks.push(block)); + } + + after + } + + /// Calls [`create_blocks`] and returns this first block reached (i.e. the last + /// block). + fn append_blocks(&mut self, stmts: &'stmt [Stmt], after: Option) -> BlockIndex { + assert!(!stmts.is_empty()); + self.create_blocks(stmts, after) + .expect("Expect `create_blocks` to create a block if `stmts` is not empty") + } + + /// If `stmts` is not empty this calls [`create_blocks`] and returns this first + /// block reached (i.e. the last block). If `stmts` is empty this returns + /// `after` and doesn't change `blocks`. + fn append_blocks_if_not_empty( + &mut self, + stmts: &'stmt [Stmt], + after: BlockIndex, + ) -> BlockIndex { + if stmts.is_empty() { + after // Empty body, continue with block `after` it. + } else { + self.append_blocks(stmts, Some(after)) + } + } + + /// Select the next block from `blocks` unconditonally. + fn unconditional_next_block(&self, after: Option) -> NextBlock<'static> { + if let Some(after) = after { + return NextBlock::Always(after); + } + + // Either we continue with the next block (that is the last block `blocks`). + // Or it's the last statement, thus we terminate. + self.blocks + .last_index() + .map_or(NextBlock::Terminate, NextBlock::Always) + } + + /// Select the next block index from `blocks`. If there is no next block it will + /// add a fake/empty block. + fn force_next_block_index(&mut self) -> BlockIndex { + self.maybe_next_block_index(None, || true) + } + + /// Select the next block index from `blocks`. If there is no next block it will + /// add a fake/empty block if `condition` returns true. If `condition` returns + /// false the returned index may not be used. + fn maybe_next_block_index( + &mut self, + after: Option, + condition: impl FnOnce() -> bool, + ) -> BlockIndex { + if let Some(after) = after { + // Next block is already determined. + after + } else if let Some(idx) = self.blocks.last_index() { + // Otherwise we either continue with the next block (that is the last + // block in `blocks`). + idx + } else if condition() { + // Or if there are no blocks, but need one based on `condition` than we + // add a fake end block. + self.blocks.push(BasicBlock::EMPTY) + } else { + // NOTE: invalid, but because `condition` returned false this shouldn't + // be used. This only used as an optimisation to avoid adding fake end + // blocks. + BlockIndex::MAX + } + } + + /// Returns a block index for a fake exception block in `blocks`. + fn fake_exception_block_index(&mut self) -> BlockIndex { + for (i, block) in self.blocks.iter_enumerated() { + if block.is_exception() { + return i; + } + } + self.blocks.push(BasicBlock::EXCEPTION) + } + + /// Change the next basic block for the block, or chain of blocks, in index + /// `fixup_index` from `from` to `to`. + /// + /// This doesn't change the target if it's `NextBlock::Terminate`. + fn change_next_block( + &mut self, + mut fixup_index: BlockIndex, + from: BlockIndex, + to: BlockIndex, + check_condition: impl Fn(&BasicBlock) -> bool + Copy, + ) { + /// Check if we found our target and if `check_condition` is met. + fn is_target( + block: &BasicBlock<'_>, + got: BlockIndex, + expected: BlockIndex, + check_condition: impl Fn(&BasicBlock) -> bool, + ) -> bool { + got == expected && check_condition(block) + } + + loop { + match self.blocks.get(fixup_index).map(|b| &b.next) { + Some(NextBlock::Always(next)) => { + let next = *next; + if is_target(&self.blocks[fixup_index], next, from, check_condition) { + // Found our target, change it. + self.blocks[fixup_index].next = NextBlock::Always(to); + } + return; + } + Some(NextBlock::If { + condition, + next, + orelse, + }) => { + let idx = fixup_index; + let condition = condition.clone(); + let next = *next; + let orelse = *orelse; + let new_next = if is_target(&self.blocks[idx], next, from, check_condition) { + // Found our target in the next branch, change it (below). + Some(to) + } else { + // Follow the chain. + fixup_index = next; + None + }; + + let new_orelse = if is_target(&self.blocks[idx], orelse, from, check_condition) + { + // Found our target in the else branch, change it (below). + Some(to) + } else if new_next.is_none() { + // If we done with the next branch we only continue with the + // else branch. + fixup_index = orelse; + None + } else { + // If we're not done with the next and else branches we need + // to deal with the else branch before deal with the next + // branch (in the next iteration). + self.change_next_block(orelse, from, to, check_condition); + None + }; + + let (next, orelse) = match (new_next, new_orelse) { + (Some(new_next), Some(new_orelse)) => (new_next, new_orelse), + (Some(new_next), None) => (new_next, orelse), + (None, Some(new_orelse)) => (next, new_orelse), + (None, None) => continue, // Not changing anything. + }; + + self.blocks[idx].next = NextBlock::If { + condition, + next, + orelse, + }; + } + Some(NextBlock::Terminate) | None => return, + } + } + } + + fn finish(mut self) -> BasicBlocks<'stmt> { + if self.blocks.is_empty() { + self.blocks.push(BasicBlock::EMPTY); + } + + BasicBlocks { + blocks: self.blocks, + } + } +} + +impl<'stmt> std::ops::Deref for BasicBlocksBuilder<'stmt> { + type Target = IndexSlice>; + + fn deref(&self) -> &Self::Target { + &self.blocks + } +} + +impl<'stmt> std::ops::DerefMut for BasicBlocksBuilder<'stmt> { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.blocks + } +} + +/// Returns true if `stmts` need a next block, false otherwise. +fn needs_next_block(stmts: &[Stmt]) -> bool { + // No statements, we automatically continue with the next block. + let Some(last) = stmts.last() else { + return true; + }; + + match last { + Stmt::Return(_) | Stmt::Raise(_) => false, + Stmt::If(stmt) => needs_next_block(&stmt.body) || needs_next_block(&stmt.orelse), + Stmt::FunctionDef(_) + | Stmt::AsyncFunctionDef(_) + | Stmt::Import(_) + | Stmt::ImportFrom(_) + | Stmt::ClassDef(_) + | Stmt::Global(_) + | Stmt::Nonlocal(_) + | Stmt::Delete(_) + | Stmt::Assign(_) + | Stmt::AugAssign(_) + | Stmt::AnnAssign(_) + | Stmt::Expr(_) + | Stmt::Pass(_) + // TODO: check below. + | Stmt::Break(_) + | Stmt::Continue(_) + | Stmt::For(_) + | Stmt::AsyncFor(_) + | Stmt::While(_) + | Stmt::With(_) + | Stmt::AsyncWith(_) + | Stmt::Match(_) + | Stmt::Try(_) + | Stmt::TryStar(_) + | Stmt::Assert(_) => true, + } +} + +/// Returns true if `stmt` contains a control flow statement, e.g. an `if` or +/// `return` statement. +fn is_control_flow_stmt(stmt: &Stmt) -> bool { + match stmt { + Stmt::FunctionDef(_) + | Stmt::AsyncFunctionDef(_) + | Stmt::Import(_) + | Stmt::ImportFrom(_) + | Stmt::ClassDef(_) + | Stmt::Global(_) + | Stmt::Nonlocal(_) + | Stmt::Delete(_) + | Stmt::Assign(_) + | Stmt::AugAssign(_) + | Stmt::AnnAssign(_) + | Stmt::Expr(_) + | Stmt::Pass(_) => false, + Stmt::Return(_) + | Stmt::For(_) + | Stmt::AsyncFor(_) + | Stmt::While(_) + | Stmt::If(_) + | Stmt::With(_) + | Stmt::AsyncWith(_) + | Stmt::Match(_) + | Stmt::Raise(_) + | Stmt::Try(_) + | Stmt::TryStar(_) + | Stmt::Assert(_) + | Stmt::Break(_) + | Stmt::Continue(_) => true, + } +} + +/// Type to create a Mermaid graph. +/// +/// To learn amount Mermaid see , for the syntax +/// see . +struct MermaidGraph<'stmt, 'source> { + graph: &'stmt BasicBlocks<'stmt>, + source: &'source str, +} + +impl<'stmt, 'source> fmt::Display for MermaidGraph<'stmt, 'source> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + // Flowchart type of graph, top down. + writeln!(f, "flowchart TD")?; + + // List all blocks. + writeln!(f, " start((\"Start\"))")?; + writeln!(f, " return((\"End\"))")?; + for (i, block) in self.graph.blocks.iter().enumerate() { + let (open, close) = if block.is_sentinel() { + ("[[", "]]") + } else { + ("[", "]") + }; + write!(f, " block{i}{open}\"")?; + if block.is_empty() { + write!(f, "`*(empty)*`")?; + } else if block.is_exception() { + write!(f, "Exception raised")?; + } else { + for stmt in block.stmts { + let code_line = &self.source[stmt.range()].trim(); + mermaid_write_quoted_str(f, code_line)?; + write!(f, "\\n")?; + } + } + writeln!(f, "\"{close}")?; + } + writeln!(f)?; + + // Then link all the blocks. + writeln!(f, " start --> block{}", self.graph.blocks.len() - 1)?; + for (i, block) in self.graph.blocks.iter_enumerated().rev() { + let i = i.as_u32(); + match &block.next { + NextBlock::Always(target) => { + writeln!(f, " block{i} --> block{target}", target = target.as_u32())?; + } + NextBlock::If { + condition, + next, + orelse, + } => { + let condition_code = &self.source[condition.range()].trim(); + writeln!( + f, + " block{i} -- \"{condition_code}\" --> block{next}", + next = next.as_u32() + )?; + writeln!( + f, + " block{i} -- \"else\" --> block{orelse}", + orelse = orelse.as_u32() + )?; + } + NextBlock::Terminate => writeln!(f, " block{i} --> return")?, + } + } + + Ok(()) + } +} + +/// Escape double quotes (`"`) in `value` using `#quot;`. +fn mermaid_write_quoted_str(f: &mut fmt::Formatter<'_>, value: &str) -> fmt::Result { + let mut parts = value.split('"'); + if let Some(v) = parts.next() { + write!(f, "{v}")?; + } + for v in parts { + write!(f, "#quot;{v}")?; + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use std::fs; + use std::path::PathBuf; + + use rustpython_parser::ast::Ranged; + use rustpython_parser::{parse, Mode}; + use std::fmt::Write; + use test_case::test_case; + + use crate::rules::ruff::rules::unreachable::{ + BasicBlocks, BlockIndex, MermaidGraph, NextBlock, + }; + + #[test_case("simple.py")] + #[test_case("if.py")] + #[test_case("while.py")] + #[test_case("for.py")] + #[test_case("async-for.py")] + //#[test_case("try.py")] // TODO. + #[test_case("raise.py")] + #[test_case("assert.py")] + #[test_case("match.py")] + fn control_flow_graph(filename: &str) { + let path = PathBuf::from_iter(["resources/test/fixtures/control-flow-graph", filename]); + let source = fs::read_to_string(&path).expect("failed to read file"); + let stmts = parse(&source, Mode::Module, filename) + .unwrap_or_else(|err| panic!("failed to parse source: '{source}': {err}")) + .expect_module() + .body; + + let mut output = String::new(); + + for (i, stmts) in stmts.into_iter().enumerate() { + let Some(func) = stmts.function_def_stmt() else { + use std::io::Write; + let _ = std::io::stderr().write_all(b"unexpected statement kind, ignoring"); + continue; + }; + + let got = BasicBlocks::from(&*func.body); + // Basic sanity checks. + assert!(!got.blocks.is_empty(), "basic blocks should never be empty"); + assert_eq!( + got.blocks.first().unwrap().next, + NextBlock::Terminate, + "first block should always terminate" + ); + + // All block index should be valid. + let valid = BlockIndex::from_usize(got.blocks.len()); + for block in &got.blocks { + match block.next { + NextBlock::Always(index) => assert!(index < valid, "invalid block index"), + NextBlock::If { next, orelse, .. } => { + assert!(next < valid, "invalid next block index"); + assert!(orelse < valid, "invalid orelse block index"); + } + NextBlock::Terminate => {} + } + } + + let got_mermaid = MermaidGraph { + graph: &got, + source: &source, + }; + + writeln!( + output, + "## Function {i}\n### Source\n```python\n{}\n```\n\n### Control Flow Graph\n```mermaid\n{}```\n", + &source[func.range()], + got_mermaid + ) + .unwrap(); + } + + insta::with_settings!({ + omit_expression => true, + input_file => filename, + description => "This is a Mermaid graph. You can use https://mermaid.live to visualize it as a diagram." + }, { + insta::assert_snapshot!(format!("{filename}.md"), output); + }); + } +} diff --git a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF014_RUF014.py.snap b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF014_RUF014.py.snap new file mode 100644 index 0000000000000..f2457017e323f --- /dev/null +++ b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF014_RUF014.py.snap @@ -0,0 +1,249 @@ +--- +source: crates/ruff/src/rules/ruff/mod.rs +--- +RUF014.py:3:5: RUF014 Unreachable code in after_return + | +1 | def after_return(): +2 | return "reachable" +3 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +4 | +5 | async def also_works_on_async_functions(): + | + +RUF014.py:7:5: RUF014 Unreachable code in also_works_on_async_functions + | +5 | async def also_works_on_async_functions(): +6 | return "reachable" +7 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +8 | +9 | def if_always_true(): + | + +RUF014.py:12:5: RUF014 Unreachable code in if_always_true + | +10 | if True: +11 | return "reachable" +12 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +13 | +14 | def if_always_false(): + | + +RUF014.py:16:9: RUF014 Unreachable code in if_always_false + | +14 | def if_always_false(): +15 | if False: +16 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +17 | return "reachable" + | + +RUF014.py:21:9: RUF014 Unreachable code in if_elif_always_false + | +19 | def if_elif_always_false(): +20 | if False: +21 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +22 | elif False: +23 | return "also unreachable" + | + +RUF014.py:23:9: RUF014 Unreachable code in if_elif_always_false + | +21 | return "unreachable" +22 | elif False: +23 | return "also unreachable" + | ^^^^^^^^^^^^^^^^^^^^^^^^^ RUF014 +24 | return "reachable" + | + +RUF014.py:28:9: RUF014 Unreachable code in if_elif_always_true + | +26 | def if_elif_always_true(): +27 | if False: +28 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +29 | elif True: +30 | return "reachable" + | + +RUF014.py:31:5: RUF014 Unreachable code in if_elif_always_true + | +29 | elif True: +30 | return "reachable" +31 | return "also unreachable" + | ^^^^^^^^^^^^^^^^^^^^^^^^^ RUF014 +32 | +33 | def ends_with_if(): + | + +RUF014.py:35:9: RUF014 Unreachable code in ends_with_if + | +33 | def ends_with_if(): +34 | if False: +35 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +36 | else: +37 | return "reachable" + | + +RUF014.py:42:5: RUF014 Unreachable code in infinite_loop + | +40 | while True: +41 | continue +42 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +43 | +44 | ''' TODO: we could determine these, but we don't yet. + | + +RUF014.py:75:5: RUF014 Unreachable code in match_wildcard + | +73 | case _: +74 | return "reachable" +75 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +76 | +77 | def match_case_and_wildcard(status): + | + +RUF014.py:83:5: RUF014 Unreachable code in match_case_and_wildcard + | +81 | case _: +82 | return "reachable" +83 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +84 | +85 | def raise_exception(): + | + +RUF014.py:87:5: RUF014 Unreachable code in raise_exception + | +85 | def raise_exception(): +86 | raise Exception +87 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +88 | +89 | def while_false(): + | + +RUF014.py:91:9: RUF014 Unreachable code in while_false + | +89 | def while_false(): +90 | while False: +91 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +92 | return "reachable" + | + +RUF014.py:96:9: RUF014 Unreachable code in while_false_else + | +94 | def while_false_else(): +95 | while False: +96 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +97 | else: +98 | return "reachable" + | + +RUF014.py:102:9: RUF014 Unreachable code in while_false_else_return + | +100 | def while_false_else_return(): +101 | while False: +102 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +103 | else: +104 | return "reachable" + | + +RUF014.py:105:5: RUF014 Unreachable code in while_false_else_return + | +103 | else: +104 | return "reachable" +105 | return "also unreachable" + | ^^^^^^^^^^^^^^^^^^^^^^^^^ RUF014 +106 | +107 | def while_true(): + | + +RUF014.py:110:5: RUF014 Unreachable code in while_true + | +108 | while True: +109 | return "reachable" +110 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +111 | +112 | def while_true_else(): + | + +RUF014.py:116:9: RUF014 Unreachable code in while_true_else + | +114 | return "reachable" +115 | else: +116 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +117 | +118 | def while_true_else_return(): + | + +RUF014.py:122:9: RUF014 Unreachable code in while_true_else_return + | +120 | return "reachable" +121 | else: +122 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +123 | return "also unreachable" + | + +RUF014.py:123:5: RUF014 Unreachable code in while_true_else_return + | +121 | else: +122 | return "unreachable" +123 | return "also unreachable" + | ^^^^^^^^^^^^^^^^^^^^^^^^^ RUF014 +124 | +125 | def while_false_var_i(): + | + +RUF014.py:128:9: RUF014 Unreachable code in while_false_var_i + | +126 | i = 0 +127 | while False: +128 | i += 1 + | ^^^^^^ RUF014 +129 | return i + | + +RUF014.py:135:5: RUF014 Unreachable code in while_true_var_i + | +133 | while True: +134 | i += 1 +135 | return i + | ^^^^^^^^ RUF014 +136 | +137 | def while_infinite(): + | + +RUF014.py:140:5: RUF014 Unreachable code in while_infinite + | +138 | while True: +139 | pass +140 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +141 | +142 | def while_if_true(): + | + +RUF014.py:146:5: RUF014 Unreachable code in while_if_true + | +144 | if True: +145 | return "reachable" +146 | return "unreachable" + | ^^^^^^^^^^^^^^^^^^^^ RUF014 +147 | +148 | # Test case found in the Bokeh repository that trigger a false positive. + | + + diff --git a/crates/ruff_dev/src/generate_json_schema.rs b/crates/ruff_dev/src/generate_json_schema.rs index bde0f4abdc587..d284783d9f411 100644 --- a/crates/ruff_dev/src/generate_json_schema.rs +++ b/crates/ruff_dev/src/generate_json_schema.rs @@ -61,7 +61,7 @@ mod tests { use super::{main, Args}; - #[test] + #[cfg_attr(not(feature = "unreachable-code"), test)] fn test_generate_json_schema() -> Result<()> { let mode = if env::var("RUFF_UPDATE_SCHEMA").as_deref() == Ok("1") { Mode::Write diff --git a/crates/ruff_index/src/slice.rs b/crates/ruff_index/src/slice.rs index 77401e7133a80..a6d3b033df79c 100644 --- a/crates/ruff_index/src/slice.rs +++ b/crates/ruff_index/src/slice.rs @@ -40,6 +40,11 @@ impl IndexSlice { } } + #[inline] + pub const fn first(&self) -> Option<&T> { + self.raw.first() + } + #[inline] pub const fn len(&self) -> usize { self.raw.len() @@ -63,6 +68,13 @@ impl IndexSlice { (0..self.len()).map(|n| I::new(n)) } + #[inline] + pub fn iter_enumerated( + &self, + ) -> impl DoubleEndedIterator + ExactSizeIterator + '_ { + self.raw.iter().enumerate().map(|(n, t)| (I::new(n), t)) + } + #[inline] pub fn iter_mut(&mut self) -> std::slice::IterMut<'_, T> { self.raw.iter_mut() diff --git a/crates/ruff_macros/src/newtype_index.rs b/crates/ruff_macros/src/newtype_index.rs index f6524b48a9e3d..2c1f6e14eca05 100644 --- a/crates/ruff_macros/src/newtype_index.rs +++ b/crates/ruff_macros/src/newtype_index.rs @@ -36,10 +36,11 @@ pub(super) fn generate_newtype_index(item: ItemStruct) -> syn::Result Self { - assert!(value <= Self::MAX as usize); + assert!(value <= Self::MAX_VALUE as usize); // SAFETY: // * The `value < u32::MAX` guarantees that the add doesn't overflow. @@ -49,7 +50,7 @@ pub(super) fn generate_newtype_index(item: ItemStruct) -> syn::Result Self { - assert!(value <= Self::MAX); + assert!(value <= Self::MAX_VALUE); // SAFETY: // * The `value < u32::MAX` guarantees that the add doesn't overflow. From 521e6de2c8833f0229f3d51ee71fd70abd5805b8 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 4 Jul 2023 14:01:29 -0400 Subject: [PATCH 06/15] Fix eval detection for suspicious-eval-usage (#5506) Closes https://github.com/astral-sh/ruff/issues/5505. --- .../test/fixtures/flake8_bandit/S307.py | 12 ++++++++++ crates/ruff/src/checkers/ast/mod.rs | 4 +--- crates/ruff/src/rules/flake8_bandit/mod.rs | 1 + .../rules/flake8_bandit/rules/exec_used.rs | 22 ++++++++++++------- .../rules/suspicious_function_call.rs | 4 ++-- ...s__flake8_bandit__tests__S102_S102.py.snap | 4 ++-- ...s__flake8_bandit__tests__S307_S307.py.snap | 20 +++++++++++++++++ 7 files changed, 52 insertions(+), 15 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/flake8_bandit/S307.py create mode 100644 crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S307_S307.py.snap diff --git a/crates/ruff/resources/test/fixtures/flake8_bandit/S307.py b/crates/ruff/resources/test/fixtures/flake8_bandit/S307.py new file mode 100644 index 0000000000000..06bccc084a803 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_bandit/S307.py @@ -0,0 +1,12 @@ +import os + +print(eval("1+1")) # S307 +print(eval("os.getcwd()")) # S307 + + +class Class(object): + def eval(self): + print("hi") + + def foo(self): + self.eval() # OK diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index c4f3617120aa5..67752031eb2d1 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2584,9 +2584,7 @@ where flake8_pie::rules::unnecessary_dict_kwargs(self, expr, keywords); } if self.enabled(Rule::ExecBuiltin) { - if let Some(diagnostic) = flake8_bandit::rules::exec_used(expr, func) { - self.diagnostics.push(diagnostic); - } + flake8_bandit::rules::exec_used(self, func); } if self.enabled(Rule::BadFilePermissions) { flake8_bandit::rules::bad_file_permissions(self, func, args, keywords); diff --git a/crates/ruff/src/rules/flake8_bandit/mod.rs b/crates/ruff/src/rules/flake8_bandit/mod.rs index 4abd69f58af28..87d0e449eb32f 100644 --- a/crates/ruff/src/rules/flake8_bandit/mod.rs +++ b/crates/ruff/src/rules/flake8_bandit/mod.rs @@ -39,6 +39,7 @@ mod tests { #[test_case(Rule::SubprocessPopenWithShellEqualsTrue, Path::new("S602.py"))] #[test_case(Rule::SubprocessWithoutShellEqualsTrue, Path::new("S603.py"))] #[test_case(Rule::SuspiciousPickleUsage, Path::new("S301.py"))] + #[test_case(Rule::SuspiciousEvalUsage, Path::new("S307.py"))] #[test_case(Rule::SuspiciousTelnetUsage, Path::new("S312.py"))] #[test_case(Rule::TryExceptContinue, Path::new("S112.py"))] #[test_case(Rule::TryExceptPass, Path::new("S110.py"))] diff --git a/crates/ruff/src/rules/flake8_bandit/rules/exec_used.rs b/crates/ruff/src/rules/flake8_bandit/rules/exec_used.rs index 3ff3db8dedc87..d2dfb83fb5c1a 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/exec_used.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/exec_used.rs @@ -1,8 +1,10 @@ -use rustpython_parser::ast::{self, Expr, Ranged}; +use rustpython_parser::ast::{Expr, Ranged}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use crate::checkers::ast::Checker; + #[violation] pub struct ExecBuiltin; @@ -14,12 +16,16 @@ impl Violation for ExecBuiltin { } /// S102 -pub(crate) fn exec_used(expr: &Expr, func: &Expr) -> Option { - let Expr::Name(ast::ExprName { id, .. }) = func else { - return None; - }; - if id != "exec" { - return None; +pub(crate) fn exec_used(checker: &mut Checker, func: &Expr) { + if checker + .semantic() + .resolve_call_path(func) + .map_or(false, |call_path| { + matches!(call_path.as_slice(), ["" | "builtin", "exec"]) + }) + { + checker + .diagnostics + .push(Diagnostic::new(ExecBuiltin, func.range())); } - Some(Diagnostic::new(ExecBuiltin, expr.range())) } diff --git a/crates/ruff/src/rules/flake8_bandit/rules/suspicious_function_call.rs b/crates/ruff/src/rules/flake8_bandit/rules/suspicious_function_call.rs index cfbb0df50c2f8..6a2add760d9c2 100644 --- a/crates/ruff/src/rules/flake8_bandit/rules/suspicious_function_call.rs +++ b/crates/ruff/src/rules/flake8_bandit/rules/suspicious_function_call.rs @@ -219,7 +219,7 @@ impl Violation for SuspiciousFTPLibUsage { } } -/// S001 +/// S301, S302, S303, S304, S305, S306, S307, S308, S310, S311, S312, S313, S314, S315, S316, S317, S318, S319, S320, S321, S323 pub(crate) fn suspicious_function_call(checker: &mut Checker, expr: &Expr) { let Expr::Call(ast::ExprCall { func, .. }) = expr else { return; @@ -246,7 +246,7 @@ pub(crate) fn suspicious_function_call(checker: &mut Checker, expr: &Expr) { // Mktemp ["tempfile", "mktemp"] => Some(SuspiciousMktempUsage.into()), // Eval - ["eval"] => Some(SuspiciousEvalUsage.into()), + ["" | "builtins", "eval"] => Some(SuspiciousEvalUsage.into()), // MarkSafe ["django", "utils", "safestring", "mark_safe"] => Some(SuspiciousMarkSafeUsage.into()), // URLOpen diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S102_S102.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S102_S102.py.snap index ccf9572377b45..075092cedae08 100644 --- a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S102_S102.py.snap +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S102_S102.py.snap @@ -6,7 +6,7 @@ S102.py:3:5: S102 Use of `exec` detected 1 | def fn(): 2 | # Error 3 | exec('x = 2') - | ^^^^^^^^^^^^^ S102 + | ^^^^ S102 4 | 5 | exec('y = 3') | @@ -16,7 +16,7 @@ S102.py:5:1: S102 Use of `exec` detected 3 | exec('x = 2') 4 | 5 | exec('y = 3') - | ^^^^^^^^^^^^^ S102 + | ^^^^ S102 | diff --git a/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S307_S307.py.snap b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S307_S307.py.snap new file mode 100644 index 0000000000000..f5c6ac82d8d12 --- /dev/null +++ b/crates/ruff/src/rules/flake8_bandit/snapshots/ruff__rules__flake8_bandit__tests__S307_S307.py.snap @@ -0,0 +1,20 @@ +--- +source: crates/ruff/src/rules/flake8_bandit/mod.rs +--- +S307.py:3:7: S307 Use of possibly insecure function; consider using `ast.literal_eval` + | +1 | import os +2 | +3 | print(eval("1+1")) # S307 + | ^^^^^^^^^^^ S307 +4 | print(eval("os.getcwd()")) # S307 + | + +S307.py:4:7: S307 Use of possibly insecure function; consider using `ast.literal_eval` + | +3 | print(eval("1+1")) # S307 +4 | print(eval("os.getcwd()")) # S307 + | ^^^^^^^^^^^^^^^^^^^ S307 + | + + From 75da72bd7fe786523f94064a2639096b61cb370f Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 4 Jul 2023 14:06:01 -0400 Subject: [PATCH 07/15] Update documentation to list double-quote preference first (#5507) Closes https://github.com/astral-sh/ruff/issues/5496. --- .../rules/flake8_quotes/rules/from_tokens.rs | 20 +++++++++---------- .../ruff/src/rules/flake8_quotes/settings.rs | 4 ++-- ruff.schema.json | 8 ++++---- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/crates/ruff/src/rules/flake8_quotes/rules/from_tokens.rs b/crates/ruff/src/rules/flake8_quotes/rules/from_tokens.rs index 6f0000e92c6da..710dec32cc0b5 100644 --- a/crates/ruff/src/rules/flake8_quotes/rules/from_tokens.rs +++ b/crates/ruff/src/rules/flake8_quotes/rules/from_tokens.rs @@ -42,16 +42,16 @@ impl AlwaysAutofixableViolation for BadQuotesInlineString { fn message(&self) -> String { let BadQuotesInlineString { quote } = self; match quote { - Quote::Single => format!("Double quotes found but single quotes preferred"), Quote::Double => format!("Single quotes found but double quotes preferred"), + Quote::Single => format!("Double quotes found but single quotes preferred"), } } fn autofix_title(&self) -> String { let BadQuotesInlineString { quote } = self; match quote { - Quote::Single => "Replace double quotes with single quotes".to_string(), Quote::Double => "Replace single quotes with double quotes".to_string(), + Quote::Single => "Replace double quotes with single quotes".to_string(), } } } @@ -91,16 +91,16 @@ impl AlwaysAutofixableViolation for BadQuotesMultilineString { fn message(&self) -> String { let BadQuotesMultilineString { quote } = self; match quote { - Quote::Single => format!("Double quote multiline found but single quotes preferred"), Quote::Double => format!("Single quote multiline found but double quotes preferred"), + Quote::Single => format!("Double quote multiline found but single quotes preferred"), } } fn autofix_title(&self) -> String { let BadQuotesMultilineString { quote } = self; match quote { - Quote::Single => "Replace double multiline quotes with single quotes".to_string(), Quote::Double => "Replace single multiline quotes with double quotes".to_string(), + Quote::Single => "Replace double multiline quotes with single quotes".to_string(), } } } @@ -139,16 +139,16 @@ impl AlwaysAutofixableViolation for BadQuotesDocstring { fn message(&self) -> String { let BadQuotesDocstring { quote } = self; match quote { - Quote::Single => format!("Double quote docstring found but single quotes preferred"), Quote::Double => format!("Single quote docstring found but double quotes preferred"), + Quote::Single => format!("Double quote docstring found but single quotes preferred"), } } fn autofix_title(&self) -> String { let BadQuotesDocstring { quote } = self; match quote { - Quote::Single => "Replace double quotes docstring with single quotes".to_string(), Quote::Double => "Replace single quotes docstring with double quotes".to_string(), + Quote::Single => "Replace double quotes docstring with single quotes".to_string(), } } } @@ -186,8 +186,8 @@ impl AlwaysAutofixableViolation for AvoidableEscapedQuote { const fn good_single(quote: Quote) -> char { match quote { - Quote::Single => '\'', Quote::Double => '"', + Quote::Single => '\'', } } @@ -200,22 +200,22 @@ const fn bad_single(quote: Quote) -> char { const fn good_multiline(quote: Quote) -> &'static str { match quote { - Quote::Single => "'''", Quote::Double => "\"\"\"", + Quote::Single => "'''", } } const fn good_multiline_ending(quote: Quote) -> &'static str { match quote { - Quote::Single => "'\"\"\"", Quote::Double => "\"'''", + Quote::Single => "'\"\"\"", } } const fn good_docstring(quote: Quote) -> &'static str { match quote { - Quote::Single => "'", Quote::Double => "\"", + Quote::Single => "'", } } diff --git a/crates/ruff/src/rules/flake8_quotes/settings.rs b/crates/ruff/src/rules/flake8_quotes/settings.rs index 121501065e90b..d0f377a2fbf2f 100644 --- a/crates/ruff/src/rules/flake8_quotes/settings.rs +++ b/crates/ruff/src/rules/flake8_quotes/settings.rs @@ -8,10 +8,10 @@ use ruff_macros::{CacheKey, CombineOptions, ConfigurationOptions}; #[serde(deny_unknown_fields, rename_all = "kebab-case")] #[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] pub enum Quote { - /// Use single quotes. - Single, /// Use double quotes. Double, + /// Use single quotes. + Single, } impl Default for Quote { diff --git a/ruff.schema.json b/ruff.schema.json index 785fac6520438..064c0f7ec9d2d 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1586,17 +1586,17 @@ "Quote": { "oneOf": [ { - "description": "Use single quotes.", + "description": "Use double quotes.", "type": "string", "enum": [ - "single" + "double" ] }, { - "description": "Use double quotes.", + "description": "Use single quotes.", "type": "string", "enum": [ - "double" + "single" ] } ] From c395e44bd761b34cab03e4d811af332ee0a2d49a Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 4 Jul 2023 14:21:05 -0400 Subject: [PATCH 08/15] Avoid PERF rules for iteration-dependent assignments (#5508) ## Summary We need to avoid raising "rewrite as a comprehension" violations in cases like: ```python d = defaultdict(list) for i in [1, 2, 3]: d[i].append(i**2) ``` Closes https://github.com/astral-sh/ruff/issues/5494. Closes https://github.com/astral-sh/ruff/issues/5500. --- .../test/fixtures/perflint/PERF401.py | 7 +++++ .../test/fixtures/perflint/PERF402.py | 7 +++++ .../rules/manual_list_comprehension.rs | 30 ++++++++++++------- .../rules/perflint/rules/manual_list_copy.rs | 30 ++++++++++++------- 4 files changed, 54 insertions(+), 20 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/perflint/PERF401.py b/crates/ruff/resources/test/fixtures/perflint/PERF401.py index beb3d4546c76f..12b084e2f6903 100644 --- a/crates/ruff/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff/resources/test/fixtures/perflint/PERF401.py @@ -30,3 +30,10 @@ def f(): result = [] for i in items: result.append(i) # OK + + +def f(): + items = [1, 2, 3, 4] + result = {} + for i in items: + result[i].append(i) # OK diff --git a/crates/ruff/resources/test/fixtures/perflint/PERF402.py b/crates/ruff/resources/test/fixtures/perflint/PERF402.py index 4db9a3dc524d3..55f3e08cbcec3 100644 --- a/crates/ruff/resources/test/fixtures/perflint/PERF402.py +++ b/crates/ruff/resources/test/fixtures/perflint/PERF402.py @@ -17,3 +17,10 @@ def f(): result = [] for i in items: result.append(i * i) # OK + + +def f(): + items = [1, 2, 3, 4] + result = {} + for i in items: + result[i].append(i * i) # OK diff --git a/crates/ruff/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff/src/rules/perflint/rules/manual_list_comprehension.rs index eb6e56735bd34..7e9607d591069 100644 --- a/crates/ruff/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff/src/rules/perflint/rules/manual_list_comprehension.rs @@ -2,6 +2,7 @@ use rustpython_parser::ast::{self, Expr, Stmt}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::any_over_expr; use crate::checkers::ast::Checker; @@ -52,6 +53,10 @@ impl Violation for ManualListComprehension { /// PERF401 pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, body: &[Stmt]) { + let Expr::Name(ast::ExprName { id, .. }) = target else { + return; + }; + let (stmt, conditional) = match body { // ```python // for x in y: @@ -99,22 +104,27 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, bo // Ignore direct list copies (e.g., `for x in y: filtered.append(x)`). if !conditional { - if arg.as_name_expr().map_or(false, |arg| { - target - .as_name_expr() - .map_or(false, |target| arg.id == target.id) - }) { + if arg.as_name_expr().map_or(false, |arg| arg.id == *id) { return; } } - let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func.as_ref() else { + let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() else { return; }; - if attr.as_str() == "append" { - checker - .diagnostics - .push(Diagnostic::new(ManualListComprehension, *range)); + if attr.as_str() != "append" { + return; + } + + // Avoid, e.g., `for x in y: filtered[x].append(x * x)`. + if any_over_expr(value, &|expr| { + expr.as_name_expr().map_or(false, |expr| expr.id == *id) + }) { + return; } + + checker + .diagnostics + .push(Diagnostic::new(ManualListComprehension, *range)); } diff --git a/crates/ruff/src/rules/perflint/rules/manual_list_copy.rs b/crates/ruff/src/rules/perflint/rules/manual_list_copy.rs index 13554488bd233..3752c4eb1c5b9 100644 --- a/crates/ruff/src/rules/perflint/rules/manual_list_copy.rs +++ b/crates/ruff/src/rules/perflint/rules/manual_list_copy.rs @@ -2,6 +2,7 @@ use rustpython_parser::ast::{self, Expr, Stmt}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::any_over_expr; use crate::checkers::ast::Checker; @@ -45,6 +46,10 @@ impl Violation for ManualListCopy { /// PERF402 pub(crate) fn manual_list_copy(checker: &mut Checker, target: &Expr, body: &[Stmt]) { + let Expr::Name(ast::ExprName { id, .. }) = target else { + return; + }; + let [stmt] = body else { return; }; @@ -72,21 +77,26 @@ pub(crate) fn manual_list_copy(checker: &mut Checker, target: &Expr, body: &[Stm }; // Only flag direct list copies (e.g., `for x in y: filtered.append(x)`). - if !arg.as_name_expr().map_or(false, |arg| { - target - .as_name_expr() - .map_or(false, |target| arg.id == target.id) - }) { + if !arg.as_name_expr().map_or(false, |arg| arg.id == *id) { return; } - let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func.as_ref() else { + let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() else { return; }; - if matches!(attr.as_str(), "append" | "insert") { - checker - .diagnostics - .push(Diagnostic::new(ManualListCopy, *range)); + if !matches!(attr.as_str(), "append" | "insert") { + return; + } + + // Avoid, e.g., `for x in y: filtered[x].append(x * x)`. + if any_over_expr(value, &|expr| { + expr.as_name_expr().map_or(false, |expr| expr.id == *id) + }) { + return; } + + checker + .diagnostics + .push(Diagnostic::new(ManualListCopy, *range)); } From 0e67757edbbf28c3a49dcc92c5844c1f432d1b51 Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Tue, 4 Jul 2023 19:49:43 +0100 Subject: [PATCH 09/15] [`pylint`] Implement Pylint `typevar-name-mismatch` (`C0132`) (#5501) ## Summary Implement Pylint `typevar-name-mismatch` (`C0132`) as `type-param-name-mismatch` (`PLC0132`). Includes documentation. Related to #970. The Pylint implementation checks only `TypeVar`, but this PR checks `TypeVarTuple`, `ParamSpec`, and `NewType` as well. This seems to better represent the Pylint rule's [intended behaviour](https://github.com/pylint-dev/pylint/issues/5224). Full disclosure: I am not a fan of the translated name and think it should probably be different. ## Test Plan `cargo test` --- .../pylint/type_param_name_mismatch.py | 56 ++++++ crates/ruff/src/checkers/ast/mod.rs | 3 + crates/ruff/src/codes.rs | 1 + crates/ruff/src/rules/pylint/mod.rs | 1 + crates/ruff/src/rules/pylint/rules/mod.rs | 2 + .../pylint/rules/type_param_name_mismatch.rs | 166 ++++++++++++++++++ ...__PLC0132_type_param_name_mismatch.py.snap | 76 ++++++++ ruff.schema.json | 3 + 8 files changed, 308 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/pylint/type_param_name_mismatch.py create mode 100644 crates/ruff/src/rules/pylint/rules/type_param_name_mismatch.rs create mode 100644 crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC0132_type_param_name_mismatch.py.snap diff --git a/crates/ruff/resources/test/fixtures/pylint/type_param_name_mismatch.py b/crates/ruff/resources/test/fixtures/pylint/type_param_name_mismatch.py new file mode 100644 index 0000000000000..267ae9a38b34f --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/type_param_name_mismatch.py @@ -0,0 +1,56 @@ +from typing import TypeVar, ParamSpec, NewType, TypeVarTuple + +# Errors. + +X = TypeVar("T") +X = TypeVar(name="T") + +Y = ParamSpec("T") +Y = ParamSpec(name="T") + +Z = NewType("T", int) +Z = NewType(name="T", tp=int) + +Ws = TypeVarTuple("Ts") +Ws = TypeVarTuple(name="Ts") + +# Non-errors. + +T = TypeVar("T") +T = TypeVar(name="T") + +T = ParamSpec("T") +T = ParamSpec(name="T") + +T = NewType("T", int) +T = NewType(name="T", tp=int) + +Ts = TypeVarTuple("Ts") +Ts = TypeVarTuple(name="Ts") + +# Errors, but not covered by this rule. + +# Non-string literal name. +T = TypeVar(some_str) +T = TypeVar(name=some_str) +T = TypeVar(1) +T = TypeVar(name=1) +T = ParamSpec(some_str) +T = ParamSpec(name=some_str) +T = ParamSpec(1) +T = ParamSpec(name=1) +T = NewType(some_str, int) +T = NewType(name=some_str, tp=int) +T = NewType(1, int) +T = NewType(name=1, tp=int) +Ts = TypeVarTuple(some_str) +Ts = TypeVarTuple(name=some_str) +Ts = TypeVarTuple(1) +Ts = TypeVarTuple(name=1) + +# No names provided. +T = TypeVar() +T = ParamSpec() +T = NewType() +T = NewType(tp=int) +Ts = TypeVarTuple() diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 67752031eb2d1..1ed61170dd8ba 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -1655,6 +1655,9 @@ where self.diagnostics.push(diagnostic); } } + if self.settings.rules.enabled(Rule::TypeParamNameMismatch) { + pylint::rules::type_param_name_mismatch(self, value, targets); + } if self.is_stub { if self.any_enabled(&[ Rule::UnprefixedTypeParam, diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 5e526f599dbdd..f39106a745412 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -156,6 +156,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pyflakes, "901") => (RuleGroup::Unspecified, rules::pyflakes::rules::RaiseNotImplemented), // pylint + (Pylint, "C0132") => (RuleGroup::Unspecified, rules::pylint::rules::TypeParamNameMismatch), (Pylint, "C0205") => (RuleGroup::Unspecified, rules::pylint::rules::SingleStringSlots), (Pylint, "C0414") => (RuleGroup::Unspecified, rules::pylint::rules::UselessImportAlias), (Pylint, "C1901") => (RuleGroup::Nursery, rules::pylint::rules::CompareToEmptyString), diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index 4f5a5b2f6a20d..c1bb1ae591082 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -85,6 +85,7 @@ mod tests { Path::new("too_many_return_statements.py") )] #[test_case(Rule::TooManyStatements, Path::new("too_many_statements.py"))] + #[test_case(Rule::TypeParamNameMismatch, Path::new("type_param_name_mismatch.py"))] #[test_case( Rule::UnexpectedSpecialMethodSignature, Path::new("unexpected_special_method_signature.py") diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index 84251fb0619f8..431c3fd170c02 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -37,6 +37,7 @@ pub(crate) use too_many_arguments::*; pub(crate) use too_many_branches::*; pub(crate) use too_many_return_statements::*; pub(crate) use too_many_statements::*; +pub(crate) use type_param_name_mismatch::*; pub(crate) use unexpected_special_method_signature::*; pub(crate) use unnecessary_direct_lambda_call::*; pub(crate) use useless_else_on_loop::*; @@ -84,6 +85,7 @@ mod too_many_arguments; mod too_many_branches; mod too_many_return_statements; mod too_many_statements; +mod type_param_name_mismatch; mod unexpected_special_method_signature; mod unnecessary_direct_lambda_call; mod useless_else_on_loop; diff --git a/crates/ruff/src/rules/pylint/rules/type_param_name_mismatch.rs b/crates/ruff/src/rules/pylint/rules/type_param_name_mismatch.rs new file mode 100644 index 0000000000000..c7020475ffd59 --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/type_param_name_mismatch.rs @@ -0,0 +1,166 @@ +use std::fmt; + +use rustpython_parser::ast::{self, Constant, Expr, Ranged}; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for `TypeVar`, `TypeVarTuple`, `ParamSpec`, and `NewType` +/// definitions in which the name of the type parameter does not match the name +/// of the variable to which it is assigned. +/// +/// ## Why is this bad? +/// When defining a `TypeVar` or a related type parameter, Python allows you to +/// provide a name for the type parameter. According to [PEP 484], the name +/// provided to the `TypeVar` constructor must be equal to the name of the +/// variable to which it is assigned. +/// +/// ## Example +/// ```python +/// from typing import TypeVar +/// +/// T = TypeVar("U") +/// ``` +/// +/// Use instead: +/// ```python +/// from typing import TypeVar +/// +/// T = TypeVar("T") +/// ``` +/// +/// ## References +/// - [Python documentation: `typing` — Support for type hints](https://docs.python.org/3/library/typing.html) +/// - [PEP 484 – Type Hints: Generics](https://peps.python.org/pep-0484/#generics) +/// +/// [PEP 484]:https://peps.python.org/pep-0484/#generics +#[violation] +pub struct TypeParamNameMismatch { + kind: VarKind, + var_name: String, + param_name: String, +} + +impl Violation for TypeParamNameMismatch { + #[derive_message_formats] + fn message(&self) -> String { + let TypeParamNameMismatch { + kind, + var_name, + param_name, + } = self; + format!("`{kind}` name `{param_name}` does not match assigned variable name `{var_name}`") + } +} + +/// PLC0132 +pub(crate) fn type_param_name_mismatch(checker: &mut Checker, value: &Expr, targets: &[Expr]) { + let [target] = targets else { + return; + }; + + let Expr::Name(ast::ExprName { id: var_name, .. }) = &target else { + return; + }; + + let Some(param_name) = param_name(value) else { + return; + }; + + if var_name == param_name { + return; + } + + let Expr::Call(ast::ExprCall { func, .. }) = value else { + return; + }; + + let Some(kind) = checker + .semantic() + .resolve_call_path(func) + .and_then(|call_path| { + if checker + .semantic() + .match_typing_call_path(&call_path, "ParamSpec") + { + Some(VarKind::ParamSpec) + } else if checker + .semantic() + .match_typing_call_path(&call_path, "TypeVar") + { + Some(VarKind::TypeVar) + } else if checker + .semantic() + .match_typing_call_path(&call_path, "TypeVarTuple") + { + Some(VarKind::TypeVarTuple) + } else if checker + .semantic() + .match_typing_call_path(&call_path, "NewType") + { + Some(VarKind::NewType) + } else { + None + } + }) + else { + return; + }; + + checker.diagnostics.push(Diagnostic::new( + TypeParamNameMismatch { + kind, + var_name: var_name.to_string(), + param_name: param_name.to_string(), + }, + value.range(), + )); +} + +#[derive(Debug, PartialEq, Eq, Copy, Clone)] +enum VarKind { + TypeVar, + ParamSpec, + TypeVarTuple, + NewType, +} + +impl fmt::Display for VarKind { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + match self { + VarKind::TypeVar => fmt.write_str("TypeVar"), + VarKind::ParamSpec => fmt.write_str("ParamSpec"), + VarKind::TypeVarTuple => fmt.write_str("TypeVarTuple"), + VarKind::NewType => fmt.write_str("NewType"), + } + } +} + +/// Returns the value of the `name` parameter to, e.g., a `TypeVar` constructor. +fn param_name(value: &Expr) -> Option<&str> { + // Handle both `TypeVar("T")` and `TypeVar(name="T")`. + let call = value.as_call_expr()?; + let name_param = call + .keywords + .iter() + .find(|keyword| { + keyword + .arg + .as_ref() + .map_or(false, |keyword| keyword.as_str() == "name") + }) + .map(|keyword| &keyword.value) + .or_else(|| call.args.get(0))?; + if let Expr::Constant(ast::ExprConstant { + value: Constant::Str(name), + .. + }) = &name_param + { + Some(name) + } else { + None + } +} diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC0132_type_param_name_mismatch.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC0132_type_param_name_mismatch.py.snap new file mode 100644 index 0000000000000..44084245c2b75 --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC0132_type_param_name_mismatch.py.snap @@ -0,0 +1,76 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +--- +type_param_name_mismatch.py:5:5: PLC0132 `TypeVar` name `T` does not match assigned variable name `X` + | +3 | # Errors. +4 | +5 | X = TypeVar("T") + | ^^^^^^^^^^^^ PLC0132 +6 | X = TypeVar(name="T") + | + +type_param_name_mismatch.py:6:5: PLC0132 `TypeVar` name `T` does not match assigned variable name `X` + | +5 | X = TypeVar("T") +6 | X = TypeVar(name="T") + | ^^^^^^^^^^^^^^^^^ PLC0132 +7 | +8 | Y = ParamSpec("T") + | + +type_param_name_mismatch.py:8:5: PLC0132 `ParamSpec` name `T` does not match assigned variable name `Y` + | +6 | X = TypeVar(name="T") +7 | +8 | Y = ParamSpec("T") + | ^^^^^^^^^^^^^^ PLC0132 +9 | Y = ParamSpec(name="T") + | + +type_param_name_mismatch.py:9:5: PLC0132 `ParamSpec` name `T` does not match assigned variable name `Y` + | + 8 | Y = ParamSpec("T") + 9 | Y = ParamSpec(name="T") + | ^^^^^^^^^^^^^^^^^^^ PLC0132 +10 | +11 | Z = NewType("T", int) + | + +type_param_name_mismatch.py:11:5: PLC0132 `NewType` name `T` does not match assigned variable name `Z` + | + 9 | Y = ParamSpec(name="T") +10 | +11 | Z = NewType("T", int) + | ^^^^^^^^^^^^^^^^^ PLC0132 +12 | Z = NewType(name="T", tp=int) + | + +type_param_name_mismatch.py:12:5: PLC0132 `NewType` name `T` does not match assigned variable name `Z` + | +11 | Z = NewType("T", int) +12 | Z = NewType(name="T", tp=int) + | ^^^^^^^^^^^^^^^^^^^^^^^^^ PLC0132 +13 | +14 | Ws = TypeVarTuple("Ts") + | + +type_param_name_mismatch.py:14:6: PLC0132 `TypeVarTuple` name `Ts` does not match assigned variable name `Ws` + | +12 | Z = NewType(name="T", tp=int) +13 | +14 | Ws = TypeVarTuple("Ts") + | ^^^^^^^^^^^^^^^^^^ PLC0132 +15 | Ws = TypeVarTuple(name="Ts") + | + +type_param_name_mismatch.py:15:6: PLC0132 `TypeVarTuple` name `Ts` does not match assigned variable name `Ws` + | +14 | Ws = TypeVarTuple("Ts") +15 | Ws = TypeVarTuple(name="Ts") + | ^^^^^^^^^^^^^^^^^^^^^^^ PLC0132 +16 | +17 | # Non-errors. + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 064c0f7ec9d2d..3b2252d9875de 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2121,6 +2121,9 @@ "PL", "PLC", "PLC0", + "PLC01", + "PLC013", + "PLC0132", "PLC02", "PLC020", "PLC0205", From 0a2620164350d8bd88d0ec1d1558c52fd8df4889 Mon Sep 17 00:00:00 2001 From: konsti Date: Tue, 4 Jul 2023 21:22:00 +0200 Subject: [PATCH 10/15] Merge clippy and clippy (wasm) jobs on CI (#5447) ## Summary The clippy wasm job rarely fails if regular clippy doesn't, wasm clippy still compiles a lot of native dependencies for the proc macro and we have less CI jobs overall, so i think this an improvement to our CI. ```shell $ CARGO_TARGET_DIR=target-wasm cargo clippy -p ruff_wasm --target wasm32-unknown-unknown --all-features -j 2 -- -D warnings $ du -sh target-wasm/* 12K target-wasm/CACHEDIR.TAG 582M target-wasm/debug 268M target-wasm/wasm32-unknown-unknown ``` ## Test plan n/a --- .github/workflows/ci.yaml | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index a7a5543ce0d48..54db12d04d272 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -31,17 +31,6 @@ jobs: cargo-clippy: name: "cargo clippy" runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v3 - - name: "Install Rust toolchain" - run: | - rustup component add clippy - - uses: Swatinem/rust-cache@v2 - - run: cargo clippy --workspace --all-targets --all-features -- -D warnings - - cargo-clippy-wasm: - name: "cargo clippy (wasm)" - runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 - name: "Install Rust toolchain" @@ -49,7 +38,10 @@ jobs: rustup component add clippy rustup target add wasm32-unknown-unknown - uses: Swatinem/rust-cache@v2 - - run: cargo clippy -p ruff_wasm --target wasm32-unknown-unknown --all-features -- -D warnings + - name: "Clippy" + run: cargo clippy --workspace --all-targets --all-features -- -D warnings + - name: "Clippy (wasm)" + run: cargo clippy -p ruff_wasm --target wasm32-unknown-unknown --all-features -- -D warnings cargo-test: strategy: From 952c62310238d8bd7bb9cb42099d7ea785c0baef Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 4 Jul 2023 15:23:05 -0400 Subject: [PATCH 11/15] Avoid returning first-match for rule prefixes (#5511) Closes #5495, but there's a TODO here to improve this further. The current `from_code` implementation feels really indirect. --- crates/ruff/src/registry.rs | 10 +++++++++- crates/ruff_cli/src/commands/rule.rs | 1 - 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index d53cd6ac7e847..d1d9014bdfcd2 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -19,7 +19,13 @@ impl Rule { pub fn from_code(code: &str) -> Result { let (linter, code) = Linter::parse_code(code).ok_or(FromCodeError::Unknown)?; let prefix: RuleCodePrefix = RuleCodePrefix::parse(&linter, code)?; - Ok(prefix.rules().next().unwrap()) + let rule = prefix.rules().next().unwrap(); + // TODO(charlie): Add a method to return an individual code, rather than matching on the + // prefix. + if rule.noqa_code().to_string() != format!("{}{}", linter.common_prefix(), code) { + return Err(FromCodeError::Prefix); + } + Ok(rule) } } @@ -27,6 +33,8 @@ impl Rule { pub enum FromCodeError { #[error("unknown rule code")] Unknown, + #[error("expected a rule code (like `SIM101`), not a prefix (like `SIM` or `SIM1`)")] + Prefix, } #[derive(EnumIter, Debug, PartialEq, Eq, Clone, Hash, RuleNamespace)] diff --git a/crates/ruff_cli/src/commands/rule.rs b/crates/ruff_cli/src/commands/rule.rs index ddb8e4ff21485..9cd41e7bb2b2c 100644 --- a/crates/ruff_cli/src/commands/rule.rs +++ b/crates/ruff_cli/src/commands/rule.rs @@ -31,7 +31,6 @@ pub(crate) fn rule(rule: Rule, format: HelpFormat) -> Result<()> { output.push('\n'); output.push('\n'); - let (linter, _) = Linter::parse_code(&rule.noqa_code().to_string()).unwrap(); output.push_str(&format!("Derived from the **{}** linter.", linter.name())); output.push('\n'); output.push('\n'); From d7214e77e69c731e74f4e7cfe8d063aab9f8ebf8 Mon Sep 17 00:00:00 2001 From: Aarni Koskela Date: Tue, 4 Jul 2023 22:45:38 +0300 Subject: [PATCH 12/15] Add `ruff rule --all` subcommand (with JSON output) (#5059) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary This adds a `ruff rule --all` switch that prints out a human-readable Markdown or a machine-readable JSON document of the lint rules known to Ruff. I needed a machine-readable document of the rules [for a project](https://github.com/astral-sh/ruff/discussions/5078), and figured it could be useful for other people – or tooling! – to be able to interrogate Ruff about its arcane knowledge. The JSON output is an array of the same objects printed by `ruff rule --format=json`. ## Test Plan I ran `ruff rule --all --format=json`. I think more might be needed, but maybe a snapshot test is overkill? --- crates/ruff_cli/src/args.rs | 12 ++- crates/ruff_cli/src/commands/rule.rs | 136 +++++++++++++++++---------- crates/ruff_cli/src/lib.rs | 9 +- docs/configuration.md | 2 +- 4 files changed, 104 insertions(+), 55 deletions(-) diff --git a/crates/ruff_cli/src/args.rs b/crates/ruff_cli/src/args.rs index 0199d2f552e31..1050aafcbf084 100644 --- a/crates/ruff_cli/src/args.rs +++ b/crates/ruff_cli/src/args.rs @@ -35,11 +35,17 @@ pub struct Args { pub enum Command { /// Run Ruff on the given files or directories (default). Check(CheckArgs), - /// Explain a rule. + /// Explain a rule (or all rules). #[clap(alias = "--explain")] + #[command(group = clap::ArgGroup::new("selector").multiple(false).required(true))] Rule { - #[arg(value_parser=Rule::from_code)] - rule: Rule, + /// Rule to explain + #[arg(value_parser=Rule::from_code, group = "selector")] + rule: Option, + + /// Explain all rules + #[arg(long, conflicts_with = "rule", group = "selector")] + all: bool, /// Output format #[arg(long, value_enum, default_value = "text")] diff --git a/crates/ruff_cli/src/commands/rule.rs b/crates/ruff_cli/src/commands/rule.rs index 9cd41e7bb2b2c..a16ec4622b6f8 100644 --- a/crates/ruff_cli/src/commands/rule.rs +++ b/crates/ruff_cli/src/commands/rule.rs @@ -1,7 +1,9 @@ use std::io::{self, BufWriter, Write}; use anyhow::Result; -use serde::Serialize; +use serde::ser::SerializeSeq; +use serde::{Serialize, Serializer}; +use strum::IntoEnumIterator; use ruff::registry::{Linter, Rule, RuleNamespace}; use ruff_diagnostics::AutofixKind; @@ -11,72 +13,106 @@ use crate::args::HelpFormat; #[derive(Serialize)] struct Explanation<'a> { name: &'a str, - code: &'a str, + code: String, linter: &'a str, summary: &'a str, message_formats: &'a [&'a str], - autofix: &'a str, + autofix: String, explanation: Option<&'a str>, + nursery: bool, } -/// Explain a `Rule` to the user. -pub(crate) fn rule(rule: Rule, format: HelpFormat) -> Result<()> { - let (linter, _) = Linter::parse_code(&rule.noqa_code().to_string()).unwrap(); - let mut stdout = BufWriter::new(io::stdout().lock()); - let mut output = String::new(); +impl<'a> Explanation<'a> { + fn from_rule(rule: &'a Rule) -> Self { + let code = rule.noqa_code().to_string(); + let (linter, _) = Linter::parse_code(&code).unwrap(); + let autofix = rule.autofixable().to_string(); + Self { + name: rule.as_ref(), + code, + linter: linter.name(), + summary: rule.message_formats()[0], + message_formats: rule.message_formats(), + autofix, + explanation: rule.explanation(), + nursery: rule.is_nursery(), + } + } +} - match format { - HelpFormat::Text => { - output.push_str(&format!("# {} ({})", rule.as_ref(), rule.noqa_code())); - output.push('\n'); - output.push('\n'); +fn format_rule_text(rule: Rule) -> String { + let mut output = String::new(); + output.push_str(&format!("# {} ({})", rule.as_ref(), rule.noqa_code())); + output.push('\n'); + output.push('\n'); - output.push_str(&format!("Derived from the **{}** linter.", linter.name())); - output.push('\n'); - output.push('\n'); + let (linter, _) = Linter::parse_code(&rule.noqa_code().to_string()).unwrap(); + output.push_str(&format!("Derived from the **{}** linter.", linter.name())); + output.push('\n'); + output.push('\n'); - let autofix = rule.autofixable(); - if matches!(autofix, AutofixKind::Always | AutofixKind::Sometimes) { - output.push_str(&autofix.to_string()); - output.push('\n'); - output.push('\n'); - } + let autofix = rule.autofixable(); + if matches!(autofix, AutofixKind::Always | AutofixKind::Sometimes) { + output.push_str(&autofix.to_string()); + output.push('\n'); + output.push('\n'); + } - if rule.is_nursery() { - output.push_str(&format!( - r#"This rule is part of the **nursery**, a collection of newer lints that are + if rule.is_nursery() { + output.push_str(&format!( + r#"This rule is part of the **nursery**, a collection of newer lints that are still under development. As such, it must be enabled by explicitly selecting {}."#, - rule.noqa_code() - )); - output.push('\n'); - output.push('\n'); - } + rule.noqa_code() + )); + output.push('\n'); + output.push('\n'); + } - if let Some(explanation) = rule.explanation() { - output.push_str(explanation.trim()); - } else { - output.push_str("Message formats:"); - for format in rule.message_formats() { - output.push('\n'); - output.push_str(&format!("* {format}")); - } - } + if let Some(explanation) = rule.explanation() { + output.push_str(explanation.trim()); + } else { + output.push_str("Message formats:"); + for format in rule.message_formats() { + output.push('\n'); + output.push_str(&format!("* {format}")); + } + } + output +} + +/// Explain a `Rule` to the user. +pub(crate) fn rule(rule: Rule, format: HelpFormat) -> Result<()> { + let mut stdout = BufWriter::new(io::stdout().lock()); + match format { + HelpFormat::Text => { + writeln!(stdout, "{}", format_rule_text(rule))?; } HelpFormat::Json => { - output.push_str(&serde_json::to_string_pretty(&Explanation { - name: rule.as_ref(), - code: &rule.noqa_code().to_string(), - linter: linter.name(), - summary: rule.message_formats()[0], - message_formats: rule.message_formats(), - autofix: &rule.autofixable().to_string(), - explanation: rule.explanation(), - })?); + serde_json::to_writer_pretty(stdout, &Explanation::from_rule(&rule))?; } }; + Ok(()) +} - writeln!(stdout, "{output}")?; - +/// Explain all rules to the user. +pub(crate) fn rules(format: HelpFormat) -> Result<()> { + let mut stdout = BufWriter::new(io::stdout().lock()); + match format { + HelpFormat::Text => { + for rule in Rule::iter() { + writeln!(stdout, "{}", format_rule_text(rule))?; + writeln!(stdout)?; + } + } + HelpFormat::Json => { + let mut serializer = serde_json::Serializer::pretty(stdout); + let mut seq = serializer.serialize_seq(None)?; + for rule in Rule::iter() { + seq.serialize_element(&Explanation::from_rule(&rule))?; + } + seq.end()?; + } + } Ok(()) } diff --git a/crates/ruff_cli/src/lib.rs b/crates/ruff_cli/src/lib.rs index 90d705b2860c7..edfb43f5de3ad 100644 --- a/crates/ruff_cli/src/lib.rs +++ b/crates/ruff_cli/src/lib.rs @@ -134,7 +134,14 @@ quoting the executed command, along with the relevant file contents and `pyproje set_up_logging(&log_level)?; match command { - Command::Rule { rule, format } => commands::rule::rule(rule, format)?, + Command::Rule { rule, all, format } => { + if all { + commands::rule::rules(format)?; + } + if let Some(rule) = rule { + commands::rule::rule(rule, format)?; + } + } Command::Config { option } => return Ok(commands::config::config(option.as_deref())), Command::Linter { format } => commands::linter::linter(format)?, Command::Clean => commands::clean::clean(log_level)?, diff --git a/docs/configuration.md b/docs/configuration.md index a64bef7a50881..3575253ed72d3 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -161,7 +161,7 @@ Usage: ruff [OPTIONS] Commands: check Run Ruff on the given files or directories (default) - rule Explain a rule + rule Explain a rule (or all rules) config List or describe the available configuration options linter List all supported upstream linters clean Clear any caches in the current directory and any subdirectories From 485d997d359b3fdbaef992e634557fd953a24f75 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 4 Jul 2023 16:02:57 -0400 Subject: [PATCH 13/15] Tweak prefix match to use .all_rules() (#5512) ## Summary No behavior change, but I think this is a little cleaner. --- crates/ruff/src/codes.rs | 12 ++++++++++++ crates/ruff/src/registry.rs | 14 ++++---------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index f39106a745412..5932150ab71bc 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -14,6 +14,18 @@ use crate::rules; #[derive(PartialEq, Eq, PartialOrd, Ord)] pub struct NoqaCode(&'static str, &'static str); +impl NoqaCode { + /// Return the prefix for the [`NoqaCode`], e.g., `SIM` for `SIM101`. + pub fn prefix(&self) -> &str { + self.0 + } + + /// Return the suffix for the [`NoqaCode`], e.g., `101` for `SIM101`. + pub fn suffix(&self) -> &str { + self.1 + } +} + impl std::fmt::Debug for NoqaCode { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { std::fmt::Display::fmt(self, f) diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index d1d9014bdfcd2..29d1da806ff22 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -18,14 +18,10 @@ pub trait AsRule { impl Rule { pub fn from_code(code: &str) -> Result { let (linter, code) = Linter::parse_code(code).ok_or(FromCodeError::Unknown)?; - let prefix: RuleCodePrefix = RuleCodePrefix::parse(&linter, code)?; - let rule = prefix.rules().next().unwrap(); - // TODO(charlie): Add a method to return an individual code, rather than matching on the - // prefix. - if rule.noqa_code().to_string() != format!("{}{}", linter.common_prefix(), code) { - return Err(FromCodeError::Prefix); - } - Ok(rule) + linter + .all_rules() + .find(|rule| rule.noqa_code().suffix() == code) + .ok_or(FromCodeError::Unknown) } } @@ -33,8 +29,6 @@ impl Rule { pub enum FromCodeError { #[error("unknown rule code")] Unknown, - #[error("expected a rule code (like `SIM101`), not a prefix (like `SIM` or `SIM1`)")] - Prefix, } #[derive(EnumIter, Debug, PartialEq, Eq, Clone, Hash, RuleNamespace)] From da1c320bfa0fdae38c0c51c8fe91781fcdcd8728 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 4 Jul 2023 16:25:16 -0400 Subject: [PATCH 14/15] Add .ipynb_checkpoints, .pyenv, .pytest_cache, and .vscode to default excludes (#5513) ## Summary VS Code extensions are [recommended](https://code.visualstudio.com/docs/python/settings-reference#_linting-settings) to exclude `.vscode` and `site-packages`. Black also now omits `.vscode`, `.pytest_cache`, and `.ipynb_checkpoints` by default. Omitting `.pyenv` is similar to omitting virtual environments, but really only matters in the context of VS Code (see: https://github.com/astral-sh/ruff/discussions/5509). Closes: #5510. --- BREAKING_CHANGES.md | 36 ++++++++++++++++++++++++++++ crates/ruff/src/settings/defaults.rs | 4 ++++ 2 files changed, 40 insertions(+) diff --git a/BREAKING_CHANGES.md b/BREAKING_CHANGES.md index 6f1c0ae85f777..cc69ea34839f9 100644 --- a/BREAKING_CHANGES.md +++ b/BREAKING_CHANGES.md @@ -1,5 +1,41 @@ # Breaking Changes +## 0.0.277 + +### `.ipynb_checkpoints`, `.pyenv`, `.pytest_cache`, and `.vscode` are now excluded by default ([#5513](https://github.com/astral-sh/ruff/pull/5513)) + +Ruff maintains a list of default exclusions, which now consists of the following patterns: + +- `.bzr` +- `.direnv` +- `.eggs` +- `.git` +- `.git-rewrite` +- `.hg` +- `.ipynb_checkpoints` +- `.mypy_cache` +- `.nox` +- `.pants.d` +- `.pyenv` +- `.pytest_cache` +- `.pytype` +- `.ruff_cache` +- `.svn` +- `.tox` +- `.venv` +- `.vscode` +- `__pypackages__` +- `_build` +- `buck-out` +- `build` +- `dist` +- `node_modules` +- `venv` + +Previously, the `.ipynb_checkpoints`, `.pyenv`, `.pytest_cache`, and `.vscode` directories were not +excluded by default. This change brings Ruff's default exclusions in line with other tools like +Black. + ## 0.0.276 ### The `keep-runtime-typing` setting has been reinstated ([#5470](https://github.com/astral-sh/ruff/pull/5470)) diff --git a/crates/ruff/src/settings/defaults.rs b/crates/ruff/src/settings/defaults.rs index 987148705dbef..0893fa754326f 100644 --- a/crates/ruff/src/settings/defaults.rs +++ b/crates/ruff/src/settings/defaults.rs @@ -39,14 +39,18 @@ pub static EXCLUDE: Lazy> = Lazy::new(|| { FilePattern::Builtin(".git"), FilePattern::Builtin(".git-rewrite"), FilePattern::Builtin(".hg"), + FilePattern::Builtin(".ipynb_checkpoints"), FilePattern::Builtin(".mypy_cache"), FilePattern::Builtin(".nox"), FilePattern::Builtin(".pants.d"), + FilePattern::Builtin(".pyenv"), + FilePattern::Builtin(".pytest_cache"), FilePattern::Builtin(".pytype"), FilePattern::Builtin(".ruff_cache"), FilePattern::Builtin(".svn"), FilePattern::Builtin(".tox"), FilePattern::Builtin(".venv"), + FilePattern::Builtin(".vscode"), FilePattern::Builtin("__pypackages__"), FilePattern::Builtin("_build"), FilePattern::Builtin("buck-out"), From 324455f580813a7c7721dc1d7ef1ee7721ae3e76 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 4 Jul 2023 17:31:32 -0400 Subject: [PATCH 15/15] Bump version to 0.0.277 (#5515) --- Cargo.lock | 6 +++--- README.md | 2 +- crates/flake8_to_ruff/Cargo.toml | 2 +- crates/ruff/Cargo.toml | 2 +- crates/ruff_cli/Cargo.toml | 2 +- docs/tutorial.md | 2 +- docs/usage.md | 4 ++-- pyproject.toml | 2 +- 8 files changed, 11 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index efc811ad0e934..b949e7d4b8c66 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -746,7 +746,7 @@ dependencies = [ [[package]] name = "flake8-to-ruff" -version = "0.0.276" +version = "0.0.277" dependencies = [ "anyhow", "clap", @@ -1829,7 +1829,7 @@ dependencies = [ [[package]] name = "ruff" -version = "0.0.276" +version = "0.0.277" dependencies = [ "annotate-snippets 0.9.1", "anyhow", @@ -1927,7 +1927,7 @@ dependencies = [ [[package]] name = "ruff_cli" -version = "0.0.276" +version = "0.0.277" dependencies = [ "annotate-snippets 0.9.1", "anyhow", diff --git a/README.md b/README.md index 1a73e65b38791..4069782b61a0e 100644 --- a/README.md +++ b/README.md @@ -139,7 +139,7 @@ Ruff can also be used as a [pre-commit](https://pre-commit.com) hook: ```yaml - repo: https://github.com/astral-sh/ruff-pre-commit # Ruff version. - rev: v0.0.276 + rev: v0.0.277 hooks: - id: ruff ``` diff --git a/crates/flake8_to_ruff/Cargo.toml b/crates/flake8_to_ruff/Cargo.toml index c411d33fe672d..1324d76596d27 100644 --- a/crates/flake8_to_ruff/Cargo.toml +++ b/crates/flake8_to_ruff/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "flake8-to-ruff" -version = "0.0.276" +version = "0.0.277" description = """ Convert Flake8 configuration files to Ruff configuration files. """ diff --git a/crates/ruff/Cargo.toml b/crates/ruff/Cargo.toml index c7a401b79cde5..45f4b18c048f8 100644 --- a/crates/ruff/Cargo.toml +++ b/crates/ruff/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "ruff" -version = "0.0.276" +version = "0.0.277" publish = false authors = { workspace = true } edition = { workspace = true } diff --git a/crates/ruff_cli/Cargo.toml b/crates/ruff_cli/Cargo.toml index 212c59de104c4..ffa12956f7ad8 100644 --- a/crates/ruff_cli/Cargo.toml +++ b/crates/ruff_cli/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "ruff_cli" -version = "0.0.276" +version = "0.0.277" publish = false authors = { workspace = true } edition = { workspace = true } diff --git a/docs/tutorial.md b/docs/tutorial.md index 452bb2583e46d..404ddc26a91bf 100644 --- a/docs/tutorial.md +++ b/docs/tutorial.md @@ -242,7 +242,7 @@ This tutorial has focused on Ruff's command-line interface, but Ruff can also be ```yaml - repo: https://github.com/astral-sh/ruff-pre-commit # Ruff version. - rev: v0.0.276 + rev: v0.0.277 hooks: - id: ruff ``` diff --git a/docs/usage.md b/docs/usage.md index 4d2f32448fb8b..1a84c8eddd5f2 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -22,7 +22,7 @@ Ruff can also be used as a [pre-commit](https://pre-commit.com) hook: ```yaml - repo: https://github.com/astral-sh/ruff-pre-commit # Ruff version. - rev: v0.0.276 + rev: v0.0.277 hooks: - id: ruff ``` @@ -32,7 +32,7 @@ Or, to enable autofix: ```yaml - repo: https://github.com/astral-sh/ruff-pre-commit # Ruff version. - rev: v0.0.276 + rev: v0.0.277 hooks: - id: ruff args: [ --fix, --exit-non-zero-on-fix ] diff --git a/pyproject.toml b/pyproject.toml index b860e3162918c..789be5574eb22 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -5,7 +5,7 @@ build-backend = "maturin" [project] name = "ruff" -version = "0.0.276" +version = "0.0.277" description = "An extremely fast Python linter, written in Rust." authors = [{ name = "Charlie Marsh", email = "charlie.r.marsh@gmail.com" }] maintainers = [{ name = "Charlie Marsh", email = "charlie.r.marsh@gmail.com" }]