Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Systematically use WithDefault for PragmaOptions #6556

Merged
merged 27 commits into from
Apr 30, 2023
Merged

Conversation

andreasabel
Copy link
Member

@andreasabel andreasabel commented Mar 28, 2023

In this PR:

  • add opposites to most boolean flags via a generic pragmaFlag method and the WithDefault type
  • compute option implications at use site; e.g. --flat-split implies --cohesion; rather than implementing this as a side effect of --flat-split, we include optFlatSplit as disjunctive component in optCohesion; thus --flat-split --no-flat-split will not turn on --cohesion
  • --exact-split is now synonym of -WCoverageNoExactSplit
  • everything cubical is left untouched by this PR
  • --count-clusters is now on by default
  • CHANGELOG and docs have been updated

Also: closes #6573, closes #6574.

@andreasabel andreasabel added type: enhancement Issues and pull requests about possible improvements ux: options Issues relating to Agda's command line options refactor Changes to the code base which do not affect users (not in changelog) labels Mar 28, 2023
@andreasabel andreasabel added this to the 2.6.4 milestone Mar 28, 2023
@andreasabel andreasabel self-assigned this Mar 28, 2023
@andreasabel andreasabel force-pushed the boolopt-default branch 2 times, most recently from 7803655 to b1bbae6 Compare April 11, 2023 12:18
@andreasabel andreasabel force-pushed the boolopt-default branch 2 times, most recently from 48ffe97 to 1ca23a3 Compare April 11, 2023 17:55
@andreasabel andreasabel force-pushed the boolopt-default branch 2 times, most recently from badbe85 to 7d008d7 Compare April 25, 2023 10:16
This is mostly a refactoring, but complements the "no-" case for many
options that did not have it explicitly.

Goal is to have a "no-" version for all boolean options,
but atm some flags have side effects and checks.
Unfortunatly, the default setting must still be 'True' or 'False'
rather that the 'true' or 'false' of the 'Boolean' class,
since this class does not lift naively to the type-level.

Maybe we can figure out to write e.g.

    WithDefault' UnicodeOrAscii 'UnicodeOk

or even

    WithDefault 'UnicodeOk

instead of now

    WithDefault' UnicodeOrAscii 'True

Main obstacle is that we have to generalize `KnownBool (b :: Bool)` to
`KnownBoolean a (b :: a)` but that would require extra boilerplate,
like duplicating `Agda.Utils.TypeLits` for each 'Boolean' type.

Ideally, one would state the iso between on `Bool` and `a` (where
`IsBool a`) only once in the `IsBool` instance, not again on the type
level...

Left for future work / needs research.
Use generalized `WithDefault' a b` for `IsBool a` types.

Generalized `pragmaFlag` to `pragmaFlag'` to handle flags with side
effects like `--no-unicode`.
`--exact-split` also controls `-WCoverageNoExactSplit`.
This now happens in the post-processing (`checkPragmaOptions`).

Ideally, we would not have _two_ mechanisms for this option.

Amended: got rid of Set.alterF
@jespercockx
Copy link
Member

Also related: #5265 and #5264. I was working on these last year but unfortunately got stalled and ran out of steam.

@andreasabel andreasabel merged commit e02b752 into master Apr 30, 2023
@andreasabel andreasabel deleted the boolopt-default branch April 30, 2023 13:26
@nad
Copy link
Contributor

nad commented May 5, 2023

@andreasabel, you changed some options so that activation of "option X that only makes sense together with option Y" now implies activation of "option Y". Is this a good idea? It can lead to less typing, but one could also argue that it is more clear if certain options always have to be declared.

Currently some options are treated in this way, while others are not. For instance, --erase-record-parameters implies --erasure, but --erased-matches does not. I think --erase-record-parameters and --erased-matches should be handled in the same way, but I have not decided which variant I prefer. Should we strive for clarity or brevity?

@andreasabel
Copy link
Member Author

andreasabel commented May 5, 2023

@andreasabel, you changed some options so that activation of "option X that only makes sense together with option Y" now implies activation of "option Y". Is this a good idea?

I think if option X is "Y with additional features" then X should imply Y, rather than explicitly require Y. For the "erasure family" of options in particular, since these have "erase" already in the name.

It is a bit like in a restaurant, where you can order a beer but also just a glass, but the order of a beer will imply the order of a glass... ;-) Meaning to say that I expect from a tool some common sense (as far as possible).

Currently some options are treated in this way, while others are not. For instance, --erase-record-parameters implies --erasure, but --erased-matches does not. I think --erase-record-parameters and --erased-matches should be handled in the same way

I agree. Somehow I didn't do anything for --erased-matches because it was possible to supply it without --erasure without getting an error (unlike --erase-record-parameters which would error without --erasure). I guess this was an oversight in case of --erased-matches. I will conform it to --erase-record-parameters so that it implies --erasure.

--erased-matches is on by default, so it cannot imply --erasure, which is off by default. What can be done about it that it will internally report to be off unless --erasure is also on. (Edit: the latter does not seem helpful.)

@nad
Copy link
Contributor

nad commented May 7, 2023

--erased-matches is on by default, so it cannot imply --erasure, which is off by default.

Well, --erased-matches is on by default if you use --with-K (which is on by default), but not if you use --without-K/--cubical-compatible/--erased-cubical/--cubical:

withKFlag :: Flag PragmaOptions
withKFlag o = return $ o
{ _optWithoutK = Value False
, _optErasedMatches = Value True
}
withoutKFlag :: Flag PragmaOptions
withoutKFlag o = return $ o
{ _optWithoutK = Value True
, _optFlatSplit = setDefault False (_optFlatSplit o)
, _optErasedMatches = setDefault False (_optErasedMatches o)
}

cubicalCompatibleFlag :: Flag PragmaOptions
cubicalCompatibleFlag o =
return $ o
{ _optCubicalCompatible = Value True
, _optWithoutK = setDefault True $ _optWithoutK o
, _optFlatSplit = setDefault False $ _optFlatSplit o
, _optErasedMatches = setDefault False $ _optErasedMatches o
}
cubicalFlag
:: Cubical -- ^ Which variant of Cubical Agda?
-> Flag PragmaOptions
cubicalFlag variant o =
return $ o
{ _optCubical = Just variant
, _optCubicalCompatible = setDefault True $ _optCubicalCompatible o
, _optWithoutK = setDefault True $ _optWithoutK o
, _optTwoLevel = setDefault True $ _optTwoLevel o
, _optFlatSplit = setDefault False $ _optFlatSplit o
, _optErasedMatches = setDefault False $ _optErasedMatches o
}

@andreasabel
Copy link
Member Author

andreasabel commented May 8, 2023

Ok, I haven't looked at the without-K/cubical family in this PR.
Basically, --erasure should be implied if _optErasedMatches has Value True, meaning it has been explicitly set by the user. This is easy to facilitate.

Just for the record, agda --without-K --erased-matches is neither an error right now (requesting --erasure) nor does it imply --erasure:

{-# OPTIONS --without-K --erased-matches #-}

foo : (@0 A : Set) (x : A)  A
foo A x = x
Erasure attributes have not been enabled (use --erasure to enable them): 0

PR:

flag b = NoArg $ effect a . set field a
where a = fromBool b
def b = applyWhen (fromBool b == b0) (++ " (default)")
expl b = if b then unwords [pos, info] else fromMaybe ("do not " ++ pos) neg
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed this unwords (case null info) by bd4f880

JobPetrovcic pushed a commit to JobPetrovcic/agda that referenced this pull request Apr 12, 2024
JobPetrovcic pushed a commit to JobPetrovcic/agda that referenced this pull request Apr 12, 2024
JobPetrovcic pushed a commit to JobPetrovcic/agda that referenced this pull request Apr 12, 2024
JobPetrovcic pushed a commit to JobPetrovcic/agda that referenced this pull request Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Changes to the code base which do not affect users (not in changelog) type: enhancement Issues and pull requests about possible improvements ux: options Issues relating to Agda's command line options
Projects
None yet
3 participants