From d5ba41035bf4ea42b7d7a05e928cf1a9fc0c98c8 Mon Sep 17 00:00:00 2001 From: Ng Zhi An Date: Sun, 28 Oct 2018 11:03:58 -0700 Subject: [PATCH 1/7] Add method to apply a multi-line replacement --- src/ShellCheck/Formatter/TTY.hs | 37 +++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/ShellCheck/Formatter/TTY.hs b/src/ShellCheck/Formatter/TTY.hs index 2d6c010..bbf01d2 100644 --- a/src/ShellCheck/Formatter/TTY.hs +++ b/src/ShellCheck/Formatter/TTY.hs @@ -183,6 +183,43 @@ doReplace start end o r = in x ++ r ++ z +-- A replacement that spans multiple line is applied by: +-- 1. merging the affected lines into a single string using `unlines` +-- 2. apply the replacement as if it only spanned a single line +-- The tricky part is adjusting the end column of the replacement +-- (the end line doesn't matter because there is only one line) +-- +-- aaS <--- start of replacement (row 1 column 3) +-- bbbb +-- cEc +-- \------- end of replacement (row 3 column 2) +-- +-- a flattened string will look like: +-- +-- "aaS\nbbbb\ncEc\n" +-- +-- The column of E has to be adjusted by: +-- 1. lengths of lines to be replaced, except the end row itself +-- 2. end column of the replacement +-- 3. number of '\n' by `unlines` +-- Returns the original lines from the file with the replacement applied. +-- Multiline replacements completely overwrite new lines in the original string. +-- e.g. if the replacement spans 2 lines, but the replacement string does not +-- have a '\n', then the number of replaced lines will be 1 shorter. +replaceMultiLines fileLines rep = + let startRow = fromIntegral $ (posLine . repStartPos) rep + endRow = fromIntegral $ (posLine . repEndPos) rep + (ys, zs) = splitAt endRow fileLines + (xs, toReplaceLines) = splitAt (startRow-1) ys + lengths = fromIntegral $ sum (map length (init toReplaceLines)) + newlines = fromIntegral $ (length toReplaceLines - 1) -- for the '\n' from unlines + original = unlines toReplaceLines + startCol = ((posColumn . repStartPos) rep) + endCol = ((posColumn . repEndPos) rep + newlines + lengths) + replacedLines = (lines $ doReplace startCol endCol original (repString rep)) + in + xs ++ replacedLines ++ zs + cuteIndent :: PositionedComment -> String cuteIndent comment = replicate (fromIntegral $ colNo comment - 1) ' ' ++ From 3471ad45b18c2a70132b5ddd28652a66f4d3f398 Mon Sep 17 00:00:00 2001 From: Ng Zhi An Date: Fri, 2 Nov 2018 22:13:49 -0700 Subject: [PATCH 2/7] Smarter sorting and application of fix to handle multiple replacements --- src/ShellCheck/Analytics.hs | 6 +-- src/ShellCheck/Formatter/TTY.hs | 85 ++++++++++++++++++++------------- src/ShellCheck/Interface.hs | 5 +- 3 files changed, 57 insertions(+), 39 deletions(-) diff --git a/src/ShellCheck/Analytics.hs b/src/ShellCheck/Analytics.hs index bbd7453..e86341d 100644 --- a/src/ShellCheck/Analytics.hs +++ b/src/ShellCheck/Analytics.hs @@ -255,15 +255,13 @@ replaceStart id params n r = repString = r } replaceEnd id params n r = - -- because of the way we count columns 1-based - -- we have to offset end columns by 1 let tp = tokenPositions params (_, end) = tp Map.! id new_start = end { - posColumn = posColumn end - n + 1 + posColumn = posColumn end - n } new_end = end { - posColumn = posColumn end + 1 + posColumn = posColumn end } in newReplacement { diff --git a/src/ShellCheck/Formatter/TTY.hs b/src/ShellCheck/Formatter/TTY.hs index bbf01d2..1afeb1a 100644 --- a/src/ShellCheck/Formatter/TTY.hs +++ b/src/ShellCheck/Formatter/TTY.hs @@ -130,7 +130,7 @@ outputForFile color sys comments = do putStrLn (color "source" line) mapM_ (\c -> putStrLn (color (severityText c) $ cuteIndent c)) commentsForLine putStrLn "" - showFixedString color comments lineNum line + showFixedString color comments lineNum fileLines ) groups hasApplicableFix lineNum comment = fromMaybe False $ do @@ -141,47 +141,43 @@ hasApplicableFix lineNum comment = fromMaybe False $ do onSameLine pos = posLine pos == lineNum -- FIXME: Work correctly with multiple replacements -showFixedString color comments lineNum line = +showFixedString color comments lineNum fileLines = + let line = fileLines !! fromIntegral (lineNum - 1) in + -- need to check overlaps case filter (hasApplicableFix lineNum) comments of (first:_) -> do -- in the spirit of error prone putStrLn $ color "message" "Did you mean: " - putStrLn $ fixedString first line + putStrLn $ unlines $ fixedString first fileLines putStrLn "" _ -> return () --- need to do something smart about sorting by end index -fixedString :: PositionedComment -> String -> String -fixedString comment line = +fixedString :: PositionedComment -> [String] -> [String] +fixedString comment fileLines = + let lineNum = lineNo comment + line = fileLines !! fromIntegral (lineNum - 1) in case (pcFix comment) of - Nothing -> "" + Nothing -> [""] Just rs -> - applyReplacement (fixReplacements rs) line 0 + -- apply replacements in sorted order by end position + -- assert no overlaps, or maybe remove overlaps? + let sorted = (reverse . sort) (fixReplacements rs) + (start, end) = calculateOverlap sorted 1 1 + in + -- applyReplacement returns the full update file, we really only care about the changed lines + -- so we calculate overlapping lines using replacements + -- TODO separate this logic of printing affected lines out + -- since for some output we might want to have the full file output + drop (fromIntegral start) $ take (fromIntegral end) $ applyReplacement sorted fileLines where - applyReplacement [] s _ = s - applyReplacement (rep:xs) s offset = - let replacementString = repString rep - start = (posColumn . repStartPos) rep - end = (posColumn . repEndPos) rep - z = doReplace start end s replacementString - len_r = (fromIntegral . length) replacementString in - applyReplacement xs z (offset + (end - start) + len_r) - --- FIXME: Work correctly with tabs --- start and end comes from pos, which is 1 based --- doReplace 0 0 "1234" "A" -> "A1234" -- technically not valid --- doReplace 1 1 "1234" "A" -> "A1234" --- doReplace 1 2 "1234" "A" -> "A234" --- doReplace 3 3 "1234" "A" -> "12A34" --- doReplace 4 4 "1234" "A" -> "123A4" --- doReplace 5 5 "1234" "A" -> "1234A" -doReplace start end o r = - let si = fromIntegral (start-1) - ei = fromIntegral (end-1) - (x, xs) = splitAt si o - (y, z) = splitAt (ei - si) xs - in - x ++ r ++ z + applyReplacement [] s = s + applyReplacement (rep:xs) s = + let result = replaceMultiLines rep s + in + applyReplacement xs result + calculateOverlap [] s e = (s, e) + calculateOverlap (rep:xs) s e = + calculateOverlap xs (min s (posLine (repStartPos rep))) (max e (posLine (repEndPos rep))) -- A replacement that spans multiple line is applied by: -- 1. merging the affected lines into a single string using `unlines` @@ -206,9 +202,9 @@ doReplace start end o r = -- Multiline replacements completely overwrite new lines in the original string. -- e.g. if the replacement spans 2 lines, but the replacement string does not -- have a '\n', then the number of replaced lines will be 1 shorter. -replaceMultiLines fileLines rep = +replaceMultiLines rep fileLines = -- this can replace doReplace let startRow = fromIntegral $ (posLine . repStartPos) rep - endRow = fromIntegral $ (posLine . repEndPos) rep + endRow = fromIntegral $ (posLine . repEndPos) rep (ys, zs) = splitAt endRow fileLines (xs, toReplaceLines) = splitAt (startRow-1) ys lengths = fromIntegral $ sum (map length (init toReplaceLines)) @@ -220,6 +216,27 @@ replaceMultiLines fileLines rep = in xs ++ replacedLines ++ zs +-- FIXME: Work correctly with tabs +-- start and end comes from pos, which is 1 based +-- doReplace 0 0 "1234" "A" -> "A1234" -- technically not valid +-- doReplace 1 1 "1234" "A" -> "A1234" +-- doReplace 1 2 "1234" "A" -> "A234" +-- doReplace 3 3 "1234" "A" -> "12A34" +-- doReplace 4 4 "1234" "A" -> "123A4" +-- doReplace 5 5 "1234" "A" -> "1234A" +doReplace start end o r = + let si = fromIntegral (start-1) + ei = fromIntegral (end-1) + (x, xs) = splitAt si o + (y, z) = splitAt (ei - si) xs + in + x ++ r ++ z + +start = newPosition { posLine = 2, posColumn = 3 } +end = newPosition { posLine = 2, posColumn = 4 } +r = newReplacement { repStartPos = start, repEndPos = end, repString = "hello" } +filelines = ["first", "second", "third", "fourth"] + cuteIndent :: PositionedComment -> String cuteIndent comment = replicate (fromIntegral $ colNo comment - 1) ' ' ++ diff --git a/src/ShellCheck/Interface.hs b/src/ShellCheck/Interface.hs index 092b9e8..b473be2 100644 --- a/src/ShellCheck/Interface.hs +++ b/src/ShellCheck/Interface.hs @@ -180,7 +180,7 @@ data Position = Position { posFile :: String, -- Filename posLine :: Integer, -- 1 based source line posColumn :: Integer -- 1 based source column, where tabs are 8 -} deriving (Show, Eq, Generic, NFData) +} deriving (Show, Eq, Generic, NFData, Ord) newPosition :: Position newPosition = Position { @@ -209,6 +209,9 @@ data Replacement = Replacement { repString :: String } deriving (Show, Eq, Generic, NFData) +instance Ord Replacement where + compare r1 r2 = (repStartPos r1) `compare` (repStartPos r2) + newReplacement = Replacement { repStartPos = newPosition, repEndPos = newPosition, From bc111141f82cfb8a9648d4fa98a539854e76754f Mon Sep 17 00:00:00 2001 From: Ng Zhi An Date: Fri, 2 Nov 2018 22:25:51 -0700 Subject: [PATCH 3/7] Move fix application logic to separate module --- src/ShellCheck/Fixer.hs | 67 ++++++++++++++++++++++++++ src/ShellCheck/Formatter/TTY.hs | 85 ++++----------------------------- 2 files changed, 75 insertions(+), 77 deletions(-) create mode 100644 src/ShellCheck/Fixer.hs diff --git a/src/ShellCheck/Fixer.hs b/src/ShellCheck/Fixer.hs new file mode 100644 index 0000000..c81d072 --- /dev/null +++ b/src/ShellCheck/Fixer.hs @@ -0,0 +1,67 @@ +module ShellCheck.Fixer (applyFix , replaceMultiLines) where + +import ShellCheck.Interface + +import Data.List + +applyFix fix fileLines = + -- apply replacements in sorted order by end position + -- assert no overlaps, or maybe remove overlaps? + let sorted = (reverse . sort) (fixReplacements fix) in + applyReplacement sorted fileLines + where + applyReplacement [] s = s + applyReplacement (rep:xs) s = applyReplacement xs $ replaceMultiLines rep s + +-- A replacement that spans multiple line is applied by: +-- 1. merging the affected lines into a single string using `unlines` +-- 2. apply the replacement as if it only spanned a single line +-- The tricky part is adjusting the end column of the replacement +-- (the end line doesn't matter because there is only one line) +-- +-- aaS <--- start of replacement (row 1 column 3) +-- bbbb +-- cEc +-- \------- end of replacement (row 3 column 2) +-- +-- a flattened string will look like: +-- +-- "aaS\nbbbb\ncEc\n" +-- +-- The column of E has to be adjusted by: +-- 1. lengths of lines to be replaced, except the end row itself +-- 2. end column of the replacement +-- 3. number of '\n' by `unlines` +-- Returns the original lines from the file with the replacement applied. +-- Multiline replacements completely overwrite new lines in the original string. +-- e.g. if the replacement spans 2 lines, but the replacement string does not +-- have a '\n', then the number of replaced lines will be 1 shorter. +replaceMultiLines rep fileLines = -- this can replace doReplace + let startRow = fromIntegral $ (posLine . repStartPos) rep + endRow = fromIntegral $ (posLine . repEndPos) rep + (ys, zs) = splitAt endRow fileLines + (xs, toReplaceLines) = splitAt (startRow-1) ys + lengths = fromIntegral $ sum (map length (init toReplaceLines)) + newlines = fromIntegral $ (length toReplaceLines - 1) -- for the '\n' from unlines + original = unlines toReplaceLines + startCol = ((posColumn . repStartPos) rep) + endCol = ((posColumn . repEndPos) rep + newlines + lengths) + replacedLines = (lines $ doReplace startCol endCol original (repString rep)) + in + xs ++ replacedLines ++ zs + +-- FIXME: Work correctly with tabs +-- start and end comes from pos, which is 1 based +-- doReplace 0 0 "1234" "A" -> "A1234" -- technically not valid +-- doReplace 1 1 "1234" "A" -> "A1234" +-- doReplace 1 2 "1234" "A" -> "A234" +-- doReplace 3 3 "1234" "A" -> "12A34" +-- doReplace 4 4 "1234" "A" -> "123A4" +-- doReplace 5 5 "1234" "A" -> "1234A" +doReplace start end o r = + let si = fromIntegral (start-1) + ei = fromIntegral (end-1) + (x, xs) = splitAt si o + (y, z) = splitAt (ei - si) xs + in + x ++ r ++ z diff --git a/src/ShellCheck/Formatter/TTY.hs b/src/ShellCheck/Formatter/TTY.hs index 1afeb1a..acc1ff4 100644 --- a/src/ShellCheck/Formatter/TTY.hs +++ b/src/ShellCheck/Formatter/TTY.hs @@ -19,6 +19,7 @@ -} module ShellCheck.Formatter.TTY (format) where +import ShellCheck.Fixer import ShellCheck.Interface import ShellCheck.Formatter.Format @@ -154,88 +155,18 @@ showFixedString color comments lineNum fileLines = fixedString :: PositionedComment -> [String] -> [String] fixedString comment fileLines = - let lineNum = lineNo comment - line = fileLines !! fromIntegral (lineNum - 1) in case (pcFix comment) of Nothing -> [""] - Just rs -> - -- apply replacements in sorted order by end position - -- assert no overlaps, or maybe remove overlaps? - let sorted = (reverse . sort) (fixReplacements rs) - (start, end) = calculateOverlap sorted 1 1 - in + Just fix -> + let (start, end) = affectedRange (fixReplacements fix) in -- applyReplacement returns the full update file, we really only care about the changed lines -- so we calculate overlapping lines using replacements - -- TODO separate this logic of printing affected lines out - -- since for some output we might want to have the full file output - drop (fromIntegral start) $ take (fromIntegral end) $ applyReplacement sorted fileLines + drop start $ take end $ applyFix fix fileLines where - applyReplacement [] s = s - applyReplacement (rep:xs) s = - let result = replaceMultiLines rep s - in - applyReplacement xs result - calculateOverlap [] s e = (s, e) - calculateOverlap (rep:xs) s e = - calculateOverlap xs (min s (posLine (repStartPos rep))) (max e (posLine (repEndPos rep))) - --- A replacement that spans multiple line is applied by: --- 1. merging the affected lines into a single string using `unlines` --- 2. apply the replacement as if it only spanned a single line --- The tricky part is adjusting the end column of the replacement --- (the end line doesn't matter because there is only one line) --- --- aaS <--- start of replacement (row 1 column 3) --- bbbb --- cEc --- \------- end of replacement (row 3 column 2) --- --- a flattened string will look like: --- --- "aaS\nbbbb\ncEc\n" --- --- The column of E has to be adjusted by: --- 1. lengths of lines to be replaced, except the end row itself --- 2. end column of the replacement --- 3. number of '\n' by `unlines` --- Returns the original lines from the file with the replacement applied. --- Multiline replacements completely overwrite new lines in the original string. --- e.g. if the replacement spans 2 lines, but the replacement string does not --- have a '\n', then the number of replaced lines will be 1 shorter. -replaceMultiLines rep fileLines = -- this can replace doReplace - let startRow = fromIntegral $ (posLine . repStartPos) rep - endRow = fromIntegral $ (posLine . repEndPos) rep - (ys, zs) = splitAt endRow fileLines - (xs, toReplaceLines) = splitAt (startRow-1) ys - lengths = fromIntegral $ sum (map length (init toReplaceLines)) - newlines = fromIntegral $ (length toReplaceLines - 1) -- for the '\n' from unlines - original = unlines toReplaceLines - startCol = ((posColumn . repStartPos) rep) - endCol = ((posColumn . repEndPos) rep + newlines + lengths) - replacedLines = (lines $ doReplace startCol endCol original (repString rep)) - in - xs ++ replacedLines ++ zs - --- FIXME: Work correctly with tabs --- start and end comes from pos, which is 1 based --- doReplace 0 0 "1234" "A" -> "A1234" -- technically not valid --- doReplace 1 1 "1234" "A" -> "A1234" --- doReplace 1 2 "1234" "A" -> "A234" --- doReplace 3 3 "1234" "A" -> "12A34" --- doReplace 4 4 "1234" "A" -> "123A4" --- doReplace 5 5 "1234" "A" -> "1234A" -doReplace start end o r = - let si = fromIntegral (start-1) - ei = fromIntegral (end-1) - (x, xs) = splitAt si o - (y, z) = splitAt (ei - si) xs - in - x ++ r ++ z - -start = newPosition { posLine = 2, posColumn = 3 } -end = newPosition { posLine = 2, posColumn = 4 } -r = newReplacement { repStartPos = start, repEndPos = end, repString = "hello" } -filelines = ["first", "second", "third", "fourth"] + affectedRange rs = _affectedRange rs 1 1 + _affectedRange [] s e = (fromIntegral s, fromIntegral e) + _affectedRange (rep:xs) s e = + _affectedRange xs (min s (posLine (repStartPos rep))) (max e (posLine (repEndPos rep))) cuteIndent :: PositionedComment -> String cuteIndent comment = From 408a3b99d885387219953ae27d0309703738906f Mon Sep 17 00:00:00 2001 From: Ng Zhi An Date: Wed, 7 Nov 2018 22:06:43 -0800 Subject: [PATCH 4/7] Remove overlaps before applying replacements --- src/ShellCheck/Fixer.hs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/ShellCheck/Fixer.hs b/src/ShellCheck/Fixer.hs index c81d072..1cabb1e 100644 --- a/src/ShellCheck/Fixer.hs +++ b/src/ShellCheck/Fixer.hs @@ -6,12 +6,26 @@ import Data.List applyFix fix fileLines = -- apply replacements in sorted order by end position - -- assert no overlaps, or maybe remove overlaps? - let sorted = (reverse . sort) (fixReplacements fix) in + let sorted = (removeOverlap . reverse . sort) (fixReplacements fix) in applyReplacement sorted fileLines where applyReplacement [] s = s applyReplacement (rep:xs) s = applyReplacement xs $ replaceMultiLines rep s + -- prereq: list is already sorted by start position + removeOverlap [] = [] + removeOverlap (x:xs) = checkoverlap x xs + checkoverlap :: Replacement -> [Replacement] -> [Replacement] + checkoverlap x [] = x:[] + checkoverlap x (y:ys) = + if overlap x y then x:(removeOverlap ys) else x:y:(removeOverlap ys) + -- two position overlaps when + overlap x y = + (yStart >= xStart && yStart <= xEnd) || (yStart < xStart && yStart > xStart) + where + yStart = repStartPos y + xStart = repStartPos x + xEnd = repEndPos x + -- A replacement that spans multiple line is applied by: -- 1. merging the affected lines into a single string using `unlines` From 3403f8d75bd4de6199b51c4c8637b9ea57d435aa Mon Sep 17 00:00:00 2001 From: Ng Zhi An Date: Sun, 16 Dec 2018 11:30:50 -0800 Subject: [PATCH 5/7] Fix bug in overlap check --- src/ShellCheck/Fixer.hs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ShellCheck/Fixer.hs b/src/ShellCheck/Fixer.hs index 1cabb1e..52a991c 100644 --- a/src/ShellCheck/Fixer.hs +++ b/src/ShellCheck/Fixer.hs @@ -20,9 +20,10 @@ applyFix fix fileLines = if overlap x y then x:(removeOverlap ys) else x:y:(removeOverlap ys) -- two position overlaps when overlap x y = - (yStart >= xStart && yStart <= xEnd) || (yStart < xStart && yStart > xStart) + (yStart >= xStart && yStart < xEnd) || (yStart < xStart && yEnd > xStart) where yStart = repStartPos y + yEnd = repEndPos y xStart = repStartPos x xEnd = repEndPos x From 7d2c519d64c233962fc449ab80a287f7c2206121 Mon Sep 17 00:00:00 2001 From: Ng Zhi An Date: Sun, 16 Dec 2018 22:17:44 -0800 Subject: [PATCH 6/7] Remove spurious new line in fix message --- src/ShellCheck/Formatter/TTY.hs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/ShellCheck/Formatter/TTY.hs b/src/ShellCheck/Formatter/TTY.hs index acc1ff4..3e0a2a8 100644 --- a/src/ShellCheck/Formatter/TTY.hs +++ b/src/ShellCheck/Formatter/TTY.hs @@ -150,7 +150,6 @@ showFixedString color comments lineNum fileLines = -- in the spirit of error prone putStrLn $ color "message" "Did you mean: " putStrLn $ unlines $ fixedString first fileLines - putStrLn "" _ -> return () fixedString :: PositionedComment -> [String] -> [String] From a8d88dfe980a597add7ca07787350775131cd76c Mon Sep 17 00:00:00 2001 From: Ng Zhi An Date: Mon, 17 Dec 2018 00:19:49 -0800 Subject: [PATCH 7/7] Fix calculation of changed lines --- src/ShellCheck/Formatter/TTY.hs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/ShellCheck/Formatter/TTY.hs b/src/ShellCheck/Formatter/TTY.hs index 3e0a2a8..dfe5421 100644 --- a/src/ShellCheck/Formatter/TTY.hs +++ b/src/ShellCheck/Formatter/TTY.hs @@ -24,6 +24,7 @@ import ShellCheck.Interface import ShellCheck.Formatter.Format import Control.Monad +import Data.Ord import Data.IORef import Data.List import Data.Maybe @@ -156,16 +157,15 @@ fixedString :: PositionedComment -> [String] -> [String] fixedString comment fileLines = case (pcFix comment) of Nothing -> [""] - Just fix -> - let (start, end) = affectedRange (fixReplacements fix) in - -- applyReplacement returns the full update file, we really only care about the changed lines - -- so we calculate overlapping lines using replacements - drop start $ take end $ applyFix fix fileLines - where - affectedRange rs = _affectedRange rs 1 1 - _affectedRange [] s e = (fromIntegral s, fromIntegral e) - _affectedRange (rep:xs) s e = - _affectedRange xs (min s (posLine (repStartPos rep))) (max e (posLine (repEndPos rep))) + Just fix -> case (fixReplacements fix) of + [] -> [] + reps -> + -- applyReplacement returns the full update file, we really only care about the changed lines + -- so we calculate overlapping lines using replacements + drop start $ take end $ applyFix fix fileLines + where + start = (fromIntegral $ minimum $ map (posLine . repStartPos) reps) - 1 + end = fromIntegral $ maximum $ map (posLine . repEndPos) reps cuteIndent :: PositionedComment -> String cuteIndent comment =