implement requireSourceFieldsUsedExcept

[nimatrueway]

An implementation of requireSourceFieldsUsedExcept(_.field1, _.field2..) as described in #248

TODO/Future Work:

• move usedSourceFields out from DerivationResult state and move it to ProductToProduct class
• implement a vararg version of def ignoreUnusedField(selector, selectors*) as Mateus suggested
• add FailOnSpecifiedUnused subclass for ActionOnUnused
  • implement a new setting to DSL def requireUsedField(selector, selectors*) as Mateus suggested
  • add RequireUsedField to TransformerOverrides to add to list of fields to be checked if FailOnSpecifiedUnused is enabled

[MateuszKubuszok]

Hello, thank you for your PR!

I think it's really nice first iteration, showing that you really looked around the codebase 😄

In general, for the sake of backward compatibility and limiting the amount of new types, and flexibility I would like to suggest several things:

• separation of the general policy (IgnoreUnusedFields/FailOnUnusedFields/FailOnUnusedFieldStrict/etc) from its exceptions (IgnoreField[Path, Cfg], FailOnUnusedField[Path, Cfg]) both in DSL, type-level config and config within macros
• that would allow changing the policy globally or in the whole scope
• allowed/rejected fields could be appended one by one, matching current conventions for other customizations
  • if needed there could be 3 possible overrides: ignore field, reject unused field, remove override for field

So that would mean:

• storing field policy as a TransformerFlag similar to ImplicitTransformerPreference (allowing us to add more policiies in the future)
  • e.g. .unusedFieldPolicy(FailOnUnused)
• adding exceptions to list... maybe requiring non-empty list of overrides
  • e.g. def ignoreUnusedField(selector, selectors*)
• things would be stored in a slightly different way internally, mostly using runtimeOverrides: Vector[(Path, TransformerOverride)] which is already managing propagation of configs based on paths

Such DSL would require a but more keystrokes but it would better fit Chimney's existing conventions.

[MateuszKubuszok]

A list of paths would not be needed if instead this would apply a config, as many times as many paths there is in vararg. This would make less code to maintain in DSL and macros.

[MateuszKubuszok]

I wouldn't like to store the result of the fields check in DerivationResult - this should be handled by ProductToProduct rule only.

[MateuszKubuszok]

I wouldn't introduce a new type parameter the DSL - while users should not rely on it, and MiMa should not complain about it (type erasure) it would break source compatibility for all people who could accidentally save the inferred type somewhere.

Instead:

• the field policy should be a separate flag, working similar to ImplicitTransformerPreference
• this would allow to change the policy globally (via -Xmacro-settings) or for all derivations in scope with implicit TransformerConfiguration
• then explicit allowance/rejection of a field would be a normal TransformerOverride following existing overrides conventions
• it would work similarly to withFieldRenamed in that there would not be any runtime data related to it
• then only ProductToProductRule would be modified to check what is the current policy and trace which fields were used, generating error if needed

[MateuszKubuszok]

Not needed when the Path is applied to Overrides for every selector in vararg

[MateuszKubuszok]

Instead of having a separate value, it should be stored in runtimeOverrides under the Path key, but with a new subtype e.g. sealed trait ForFieldPolicy extends TransformerOverride

[MateuszKubuszok]

unused: Set[String] -> unusedFields: ListSet[String]

[MateuszKubuszok]

Maybe are required to be used in the transformation but are not used -> were required to be used in the transformation but are not used?

[MateuszKubuszok]

* @return
*   [[io.scalaland.chimney.dsl.TransformerInto]]
*
* @since 1.5.0

[MateuszKubuszok]

Snippet cannot at the same time check successful output and fail compilation, it would have to be 2 snippets:

• one with // expected error:
• one with expected output:

without it, snipped checking will fail.

[nimatrueway]

Hello, thank you for your PR!

I think it's really nice first iteration, showing that you really looked around the codebase 😄
...

My pleasure! Thank you so much for the quick review and good ideas. I initially had a similar thought in #601, it's nice to hear it wasn't too far off.

I'll start developing the API change 🤓🚧

[nimatrueway]

Hey @MateuszKubuszok, I addressed the architectural change you proposed (and almost all your comments). Namely I split the API into a flag and a setting to override configuration per field.

// implementation:done -> tracks all fields and forces compile error if any left unused
.enableUnusedFieldPolicy(FailOnUnused)

// implementation:done -> untracks a single field
// TODO: how do I implement the vararg version without introducing PathList type? could use your direction here
.withIgnoreUnusedField(_.b)

To avoid a API change in the future and preserve backward compatibility, it's probably best to fix the API in this PR. So I tried to formulate your vision. Here's the domain terms and phrases I imagined makes sense for this feature. Please share if you have any suggestion.

• I would like to track certain fields and fail transformation derivation if any are unused.
• I would like to track certain fields and spit out warning during transformation derivation if any are unused.
• I would like to track all fields of the source type by default.
• I would like to track none of fields of the source type by default.
• I would like to ignore certain fields if unused.
• I would like to track certain fields if unused.

In that case I imagined the flag to be parameterized on two aspects fail/warn on unused and whether to track all fields or none of the fields by default.

// TODO: does this make sense? isn't it too verbose?
.enableUnusedFieldPolicy(FailOnUnused, TrackAll)
.enableUnusedFieldPolicy(WarnOnUnused, TrackNone)

// TODO: to manually track a bunch of fields
.withTrackUnusedFields(_.b, _.c)

[MateuszKubuszok]

Hmm, the way I see it we have 2 possible policies for now:

• ignore any unused field
• fail the compilation on any field which was not explicitly marked as unused

and both would additionally fail the compilation if someone marked a field as unused but it was required.

In some more-or-less distant future, after #115, we might have additional policies:

• ignore unused fields in fallback, failing on unused fields in the main value
• fail the compilation for for any field, either in main value or fallback, what wasn't explicitly marked as unused
  • this could be problematic as I cannot think of a good DSL to mark such field as explicitly unused

Additionally, we have to keep in mind that:

• even though we say field, actually right now Chimney makes no distinction between vals defined like:

  case class Foo(a: Int, b: String)

  and:

  case class Foo() {
    val a: Int = 0
    val b: String = "text"
  }

  and, some people would probably prefer to make the distinction which they want to ignore, but that distinction would have to be implemented somewhere in the future, so for now, we would have to keep the distinction in mind when naming things, e.g. by using name like "unused source values"

• these flags are global, so whatever name we use, should not collide with whatever users' might use in their codebases, verbose names are less of an issue

• we would have to add similar thing for target subtypes (when it's an enum/a subtype), so we have to make sure that the name indicates which is which

• ATM Chimney does not support showing warnings - it's either info for logs, or error for reasons why macro cannot be expanded, we might add it in the future, as something like policyReportingStrategy(PolicyCheckFailureIsWarning)/policyReportingStrategy(PolicyCheckFailureIsError) but that would increase the scope of this PR too much.

---

Currently I think the API what would work the best would be something like:

// sealed abstract class UnusedFieldPolicy
// object FailOnIgnoredSourceVal extends UnusedFieldPolicy

src.into[Target]
.enableUnusedFieldPolicy(FailOnIgnoredSourceVal) // 1 value for now
.disableUnusedFieldPolicy // ignores unused fields, the default
.withFieldUnused(_.foo.bar.baz) // expect that field is unused:
                                // - fail compilation if it is needed
                                // - do NOT treat field as ignored when UnusedFieldPolicy is enabled
                                // - be na noop when UnusedFieldPolicy is disabled and field is not used
.transform

Then we could test it similarly to other flags:

• in both TotalTransformerProductSpec and PartialTransformerProductSpec:
• group("flag .enableUnusedFieldPolicy")
  • test("should be disabled by default")
  • test("should inform user which unused fields failed")
  • test("should ignore explicitly unused fields")
  • test("should not allow marking required fields as unused")
• group("flag .disableUnusedFieldPolicy")
• test("should disable globally enabled .enableUnusedFieldPolicy")
• tests would have to check that nesting case classes would also work

---

Adding:

• distinction between warnings and errors
• distinction between case class fields and other vals
• support for UnusedTargetSubtypePolicy

would be a job for another PR, and not necessarily something to do soon. I think that even this change would require some refactoring to how TransformationContext works with regard to Paths in source, so I did a little refactoring in #612 and #613 (sorry for the conflicts 😅 )

[nimatrueway]

Hmm, the way I see it we have 2 possible policies for now:

...

Thanks for sharing your insights Mateus, I skimmed through your comment twice and we are aligned. I need to carve up sometime on the weekend to hack it. 🚀