Raw parse error for --allow-newer=''

[Bodigrim]

Describe the bug
Passing --allow-newer='' raises a raw parse error.

To Reproduce

$ cabal --version
cabal-install version 3.6.2.0
compiled using version 3.6.2.0 of the Cabal library

$ cabal build --allow-newer=''
Cannot parse the list of packages:
CallStack (from HasCallStack):
  error, called at src/Distribution/ReadE.hs:42:24 in Cbl-3.6.2.0-feadae31:Distribution.ReadE

Expected behavior
I would expect this invokation to succeed (what's so wrong with an empty list of packages?) or at least fail with a non-internal error.

[philderbeast]

Empty list parsing

I did a little tracing of cabal repl cabal-install:cabal to get a feel for the --allow-newer option parsing. This option is described in two places in the docs; for the project and for the command line.

I found two ways of explicitly supplying an empty list.

:main build --allow-newer=
Just (RelaxDepsSome [])

:main build --allow-newer= --allow-newer=
Just (RelaxDepsSome [])

Outer enclosing quotes are fine but not inner quotes enclosing an empty list (and neither is empty list as []).

:main build "--allow-newer="
Just (RelaxDepsSome [])

:main build --allow-newer=''
Error: <interactive>: invalid argument to option `--allow-newer': Cannot parse
the list of packages: ''
*** Exception: ExitFailure 1

:main build --allow-newer=""
Error: <interactive>: invalid argument to option `--allow-newer': Cannot parse
the list of packages: ""
*** Exception: ExitFailure 1

:main build --allow-newer=[]
Error: <interactive>: invalid argument to option `--allow-newer': Cannot parse
the list of packages: []
*** Exception: ExitFailure 1

At this point the behaviour seems fine but perhaps we could update the docs with a note or warning?

Non-empty list parsing

An example for the project is allow-newer: all:bar, *:baz, *:^quux. I cannot paste this verbatim as a list for the command line (but this will work without the spaces as I find out later).

:main build "--allow-newer=all:bar, *:baz, *:^quux"
Error: <interactive>: invalid argument to option `--allow-newer': Cannot parse
the list of packages: all:bar, *:baz, *:^quux
*** Exception: ExitFailure 1

I can convert that project list example to the command line by repeating the option for each list item.

:main build --allow-newer=all:bar --allow-newer=*:baz --allow-newer=*:^quux
Just (RelaxDepsSome [RelaxedDep RelaxDepScopeAll RelaxDepModNone (RelaxDepSubjectPkg (PackageName "bar"))])
Just (RelaxDepsSome [RelaxedDep RelaxDepScopeAll RelaxDepModNone (RelaxDepSubjectPkg (PackageName "baz"))])
Just (RelaxDepsSome [RelaxedDep RelaxDepScopeAll RelaxDepModCaret (RelaxDepSubjectPkg (PackageName "quux"))])

An example for the command line shows a condensed list --allow-newer=bar,baz,quux with comma separators, no spaces.

:main build --allow-newer=bar,baz,quux
Just (RelaxDepsSome
  [ RelaxedDep RelaxDepScopeAll RelaxDepModNone (RelaxDepSubjectPkg (PackageName "bar"))
  , RelaxedDep RelaxDepScopeAll RelaxDepModNone (RelaxDepSubjectPkg (PackageName "baz"))
  , RelaxedDep RelaxDepScopeAll RelaxDepModNone (RelaxDepSubjectPkg (PackageName "quux"))
  ])

The repeated option --allow-newer= --allow-newer= is not representable in the condensed form.

:main build --allow-newer=,
Error: <interactive>: invalid argument to option `--allow-newer': Cannot parse
the list of packages: ,
*** Exception: ExitFailure 1

The docs for the project say that "A bare --allow-newer is equivalent to --allow-newer=all" and that checks out. They're parsed as separate cases but I presume there's a pattern match somewhere that handles both the same way.

:main build --allow-newer=all
Just (RelaxDepsSome [RelaxedDep RelaxDepScopeAll RelaxDepModNone RelaxDepSubjectAll])

:main build --allow-newer
Just RelaxDepsAll

The parsing of =none is handled as a string for the package name unlike the parsing for =all. There's no package named none on hackage.

:main build --allow-newer=none
Just (RelaxDepsSome [RelaxedDep RelaxDepScopeAll RelaxDepModNone (RelaxDepSubjectPkg (PackageName "none"))])

Should we add another case, RelaxDepSubjectNone?

cabal/cabal-install/src/Distribution/Client/Types/AllowNewer.hs

Lines 82 to 85 in 00a2351

	-- | Express whether to relax bounds /on/ @all@ packages, or a single package
	data RelaxDepSubject = RelaxDepSubjectAll
	| RelaxDepSubjectPkg !PackageName
	deriving (Eq, Ord, Read, Show, Generic)

cabal/cabal-install/src/Distribution/Client/Types/AllowNewer.hs

Lines 169 to 178 in 00a2351

	instance Parsec RelaxDeps where
	parsec = do
	xs <- parsecLeadingCommaNonEmpty parsec
	pure $ case toList xs of
	[RelaxedDep RelaxDepScopeAll RelaxDepModNone RelaxDepSubjectAll]
	-> RelaxDepsAll
	[RelaxedDep RelaxDepScopeAll RelaxDepModNone (RelaxDepSubjectPkg pn)]
	| pn == mkPackageName "none"
	-> mempty
	xs' -> mkRelaxDepSome xs'

[philderbeast]

With this change I could do what is asked (and more) but am not sure I should.

relaxDepsParser :: CabalParsing m => m (Maybe RelaxDeps)
relaxDepsParser =
-  (Just . RelaxDepsSome . toList) `fmap` P.sepByNonEmpty parsec (P.char ',')
+  (Just . RelaxDepsSome . const []) `fmap` (P.string "''" <|> P.string "\"\"" <|> P.string "[]")
+  <|>
+  (Just . RelaxDepsSome . toList) `fmap` P.sepBy parsec (P.string ", ")
+  <|>
+  (Just . RelaxDepsSome . toList) `fmap` P.sepBy parsec (P.char ',')

:main build --allow-newer=''
Just (RelaxDepsSome [])

:main build --allow-newer=""
Just (RelaxDepsSome [])

:main build --allow-newer=[]
Just (RelaxDepsSome [])

:main build "--allow-newer=all:bar, *:baz, *:^quux"
Just (RelaxDepsSome
    [ RelaxedDep RelaxDepScopeAll RelaxDepModNone (RelaxDepSubjectPkg (PackageName "bar"))
    , RelaxedDep RelaxDepScopeAll RelaxDepModNone (RelaxDepSubjectPkg (PackageName "baz"))
    , RelaxedDep RelaxDepScopeAll RelaxDepModCaret (RelaxDepSubjectPkg (PackageName "quux"))
    ])

[phadej]

The tricky part is that --allow-newer alone without = means allow all newer. So allowing to parse empty string is potentially confusing:

cabal build --allow-newer=
cabal build --allow-newer

would do very different things.

[Mikolaj]

I agree this is confusing. If so, should we intercept --allow-newer= and complain "the argument list for --allow-newer= should not be empty; use --allow-newer without the equals sign to permit all packages to use newer versions"?

[Mikolaj]

Hello! Let's try to reach a consensus so that our skilled contributors can implement it. Is it fine to error out at --allow-newer=?

[Bodigrim]

I agree this is confusing. If so, should we intercept --allow-newer= and complain "the argument list for --allow-newer= should not be empty; use --allow-newer without the equals sign to permit all packages to use newer versions"?

Yes, sounds reasonable to me.

[Mikolaj]

Thank you. No objections since Jan, so the PR is really welcome, with the agreed design. I imagine this can't be too hard, given the analysis above.

[ulysses4ever]

I looked into this a bit and got stuck: there doesn't seem to be a good way to tune the error message according to a parser error because of how parsecToReadE is defined:

parsecToReadE :: (String -> ErrorMsg) -> ParsecParser a -> ReadE a
parsecToReadE err p = ReadE $ \txt ->
    case runParsecParser p "<parsecToReadE>" (fieldLineStreamFromString txt) of
        Right x -> Right x
        Left _e -> Left (err txt)  -- discarding the error message from the parser!

For the context,

  , option [] ["allow-newer"]
    ("Ignore upper bounds in all dependencies or DEPS")
    ...
    (optArg "DEPS"
     (parsecToReadE ("Cannot parse the list of packages: " ++) relaxDepsParser)
     (Just RelaxDepsAll) relaxDepsPrinter)

So, the actual parsing is done in relaxDepsParser, and it can fail with an appropriate message under the desired condition, but that message will be discarded by parsecToReadE and replaced with mere "Cannot parse the list of packages: " ++ <parser input>. Any ideas how to deal with it?

[ulysses4ever]

Posted #8140 for this, but it's not ideal, I feel.

[ulysses4ever]

#8140 searches for the second reviewer, don't pass by!