deprecation: limit deep unmarking for better performance #38082

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

Open

DanielMSchmidt wants to merge 14 commits into use-deprecation-marks-for-resource-deprecation-3 from limit-usage-of-deep-unmarking-2

+148 −158

internal/deprecation/deprecation.go

-Original file line number
+Diff line change
@@ Expand Up @@
     // values.
     // If the value can not be referenced, use ValidateDeep or ValidateAsConfig instead.
     func (d *Deprecations) Validate(value cty.Value, module addrs.Module, rng *hcl.Range) (cty.Value, tfdiags.Diagnostics) {
-    	deprecationMarks := marks.GetDeprecationMarks(value)
-    	notDeprecatedValue := marks.RemoveDeprecationMarks(value)
+    	notDeprecatedValue, deprecationMarks := marks.GetDeprecationMarks(value)
     	return notDeprecatedValue, d.deprecationMarksToDiagnostics(deprecationMarks, module, rng)
     }
-    // ValidateDeep does the same as Validate but checks deeply nested deprecation marks as well.
-    func (d *Deprecations) ValidateDeep(value cty.Value, module addrs.Module, rng *hcl.Range) (cty.Value, tfdiags.Diagnostics) {
-    	deprecationMarks := marks.GetDeprecationMarksDeep(value)
-    	notDeprecatedValue := marks.RemoveDeprecationMarksDeep(value)
-    	return notDeprecatedValue, d.deprecationMarksToDiagnostics(deprecationMarks, module, rng)
+    // ValidateExpressionDeep looks for deprecation marks deeply within the given value
+    // and returns diagnostics for each deprecation found, unless deprecation warnings
+    // are suppressed for the given module. It finds the most specific range possible for
+    // each diagnostic.
+    func (d *Deprecations) ValidateExpressionDeep(value cty.Value, module addrs.Module, expr hcl.Expression) (cty.Value, tfdiags.Diagnostics) {
+    	var diags tfdiags.Diagnostics
+    	unmarked, pvms := value.UnmarkDeepWithPaths()
+    	// Check if we need to suppress deprecation warnings for this module call.
+    	if d.IsModuleCallDeprecationSuppressed(module) {
+    		return unmarked.MarkWithPaths(marks.RemoveAll(pvms, marks.Deprecation)), diags
+    	}
+    	for _, pvm := range pvms {
+    		for m := range pvm.Marks {
+    			if depMark, ok := m.(marks.DeprecationMark); ok {
+    				rng := tfdiags.RangeForExpressionAtPath(expr, pvm.Path)
+    				diags = diags.Append(deprecationMarkToDiagnostic(depMark, &rng))
+    			}
+    		}
+    	}
+    	return unmarked.MarkWithPaths(marks.RemoveAll(pvms, marks.Deprecation)), diags
     }
     func (d *Deprecations) deprecationMarksToDiagnostics(deprecationMarks []marks.DeprecationMark, module addrs.Module, rng *hcl.Range) tfdiags.Diagnostics {
@@ Expand All @@
     	}
     	for _, depMark := range deprecationMarks {
-    		diag := &hcl.Diagnostic{
-    			Severity: hcl.DiagWarning,
-    			Summary:  "Deprecated value used",
-    			Detail:   depMark.Message,
-    			Subject:  rng,
-    		}
-    		if depMark.OriginDescription != "" {
-    			diag.Extra = &tfdiags.DeprecationOriginDiagnosticExtra{
-    				OriginDescription: depMark.OriginDescription,
-    			}
-    		}
-    		diags = diags.Append(diag)
+    		diags = diags.Append(deprecationMarkToDiagnostic(depMark, rng))
     	}
     	return diags
     }
-    // ValidateAsConfig checks the given value for deprecation marks and returns diagnostics
+    func deprecationMarkToDiagnostic(depMark marks.DeprecationMark, subject *hcl.Range) *hcl.Diagnostic {
+    	diag := &hcl.Diagnostic{
+    		Severity: hcl.DiagWarning,
+    		Summary:  "Deprecated value used",
+    		Detail:   depMark.Message,
+    		Subject:  subject,
+    	}
+    	if depMark.OriginDescription != "" {
+    		diag.Extra = &tfdiags.DeprecationOriginDiagnosticExtra{
+    			OriginDescription: depMark.OriginDescription,
+    		}
+    	}
+    	return diag
+    }
+    // ValidateConfig checks the given value deeply for deprecation marks and returns diagnostics
     // for each deprecation found, unless deprecation warnings are suppressed for the given module.
-    // It checks for deeply nested deprecation marks as well.
-    func (d *Deprecations) ValidateAsConfig(value cty.Value, schema *configschema.Block, module addrs.Module) tfdiags.Diagnostics {
+    func (d *Deprecations) ValidateConfig(value cty.Value, schema *configschema.Block, module addrs.Module) (cty.Value, tfdiags.Diagnostics) {
     	var diags tfdiags.Diagnostics
-    	_, pvms := value.UnmarkDeepWithPaths()
+    	unmarked, pvms := value.UnmarkDeepWithPaths()
-    	if len(pvms) == 0 || d.IsModuleCallDeprecationSuppressed(module) {
-    		return diags
+    	if d.IsModuleCallDeprecationSuppressed(module) {
+    		// Even if we don't want to get deprecation warnings we want to remove the marks
+    		return unmarked.MarkWithPaths(marks.RemoveAll(pvms, marks.Deprecation)), diags
     	}
     	for _, pvm := range pvms {
@@ Expand Down Expand Up @@
     			}
     		}
     	}
-    	return diags
+    	return unmarked.MarkWithPaths(marks.RemoveAll(pvms, marks.Deprecation)), diags
     }
     func (d *Deprecations) IsModuleCallDeprecationSuppressed(addr addrs.Module) bool {
@@ Expand All @@
     	}
     	return false
     }
-    func (d *Deprecations) DiagnosticsForValueMarks(valueMarks cty.ValueMarks, module addrs.Module, rng *hcl.Range) tfdiags.Diagnostics {
-    	return d.deprecationMarksToDiagnostics(marks.FilterDeprecationMarks(valueMarks), module, rng)
-    }

internal/lang/marks/marks.go

-Original file line number
+Diff line change
@@ Expand Up / @@ -51,28 +51,25 @@ func Contains(val cty.Value, mark interface{}) bool { @@
     // FilterDeprecationMarks returns all deprecation marks present in the given
     // cty.ValueMarks.
-    func FilterDeprecationMarks(marks cty.ValueMarks) []DeprecationMark {
+    func FilterDeprecationMarks(marks cty.ValueMarks) (cty.ValueMarks, []DeprecationMark) {
+    	other := cty.ValueMarks{}
     	depMarks := []DeprecationMark{}
     	for mark := range marks {
     		if d, ok := mark.(DeprecationMark); ok {
     			depMarks = append(depMarks, d)
+    		} else {
+    			other[mark] = struct{}{}
     		}
     	}
-    	return depMarks
+    	return other, depMarks
     }
     // GetDeprecationMarks returns all deprecation marks present on the given
     // cty.Value.
-    func GetDeprecationMarks(val cty.Value) []DeprecationMark {
-    	_, marks := val.Unmark()
-    	return FilterDeprecationMarks(marks)
-    }
-    // GetDeprecationMarksDeep returns all deprecation marks present on the given
-    // cty.Value or any nested values.
-    func GetDeprecationMarksDeep(val cty.Value) []DeprecationMark {
-    	_, marks := val.UnmarkDeep()
-    	return FilterDeprecationMarks(marks)
+    func GetDeprecationMarks(val cty.Value) (cty.Value, []DeprecationMark) {
+    	unmarked, marks := val.Unmark()
+    	other, depMarks := FilterDeprecationMarks(marks)
+    	return unmarked.WithMarks(other), depMarks
     }
     // RemoveDeprecationMarks returns a copy of the given cty.Value with all
@@ Expand Down @@

internal/terraform/context_apply2_test.go

-Original file line number
+Diff line change
@@ Expand Up / @@ -4461,60 +4461,8 @@ resource "test_resource" "test" { @@
     				)
     			},
     			expectedDiagnostics: func(m *configs.Config) tfdiags.Diagnostics {
-    				return tfdiags.Diagnostics{}.Append(&hcl.Diagnostic{
-    					Severity: hcl.DiagWarning,
-    					Summary:  "Deprecated value used",
-    					Detail:   "Please stop using this output",
-    					Subject: &hcl.Range{
-    						Filename: filepath.Join(m.Module.SourceDir, "main.tf"),
-    						Start:    hcl.Pos{Line: 7, Column: 13, Byte: 86},
-    						End:      hcl.Pos{Line: 7, Column: 27, Byte: 100},
-    					},
-    				})
-    			},
-    		},
-    		"update resource to stop using deprecated output": {
-    			modules: map[string]string{
-    				"mod/main.tf": `
-    output "old" {
-        deprecated = "Please stop using this output"
-        value = "deprecated-value"
-    }
-    output "new" {
-        value = "new-value"
-    }
-    `,
-    				"main.tf": `
-    module "mod" {
-        source = "./mod"
-    }
-    resource "test_resource" "test" {
-        value = module.mod.old
-    }
-    `,
-    			},
-    			buildState: func(s *states.SyncState) {
-    				s.SetResourceInstanceCurrent(
-    					mustResourceInstanceAddr("test_resource.test"),
-    					&states.ResourceInstanceObjectSrc{
-    						AttrsJSON: []byte(`{"value":"deprecated-value"}`),
-    						Status:    states.ObjectReady,
-    					},
-    					mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`),
-    				)
-    			},
-    			expectedDiagnostics: func(m *configs.Config) tfdiags.Diagnostics {
-    				return tfdiags.Diagnostics{}.Append(&hcl.Diagnostic{
-    					Severity: hcl.DiagWarning,
-    					Summary:  "Deprecated value used",
-    					Detail:   "Please stop using this output",
-    					Subject: &hcl.Range{
-    						Filename: filepath.Join(m.Module.SourceDir, "main.tf"),
-    						Start:    hcl.Pos{Line: 7, Column: 13, Byte: 86},
-    						End:      hcl.Pos{Line: 7, Column: 27, Byte: 100},
-    					},
-    				})
+    				// if the resource instance is not executed, we can not warn (during plan a warning was already emitted)
+    				return tfdiags.Diagnostics{}
     			},
     		},
     		"create resource using deprecated variable": {
@@ Expand Down @@

internal/terraform/eval_conditions.go

-Original file line number
+Diff line change
@@ Expand Up @@
     	errorMessage, moreDiags := lang.EvalCheckErrorMessage(rule.ErrorMessage, hclCtx, &addr)
     	diags = diags.Append(moreDiags)
-    	_, deprecationDiags := ctx.Deprecations().ValidateDeep(errorMessage, ctx.Path().Module(), rule.ErrorMessage.Range().Ptr())
+    	_, deprecationDiags := ctx.Deprecations().Validate(errorMessage, ctx.Path().Module(), rule.ErrorMessage.Range().Ptr())
     	diags = diags.Append(deprecationDiags)
     	// NOTE: We've discarded any other marks the string might have been carrying,
@@ Expand Down Expand Up @@
     		return checkResult{Status: checks.StatusError}, diags
     	}
-    	// We don't care about the returned value here, only the diagnostics
-    	_, deprecationDiags := ctx.Deprecations().ValidateDeep(resultVal, addr.ModuleInstance().Module(), rule.Condition.Range().Ptr())
+    	resultVal, deprecationDiags := ctx.Deprecations().Validate(resultVal, addr.ModuleInstance().Module(), rule.Condition.Range().Ptr())
     	diags = diags.Append(deprecationDiags)
     	var err error
@@ Expand Down @@

internal/terraform/node_action_instance.go

-Original file line number
+Diff line change
@@ Expand Up @@
     		valDiags := validateResourceForbiddenEphemeralValues(ctx, configVal, n.Schema.ConfigSchema)
     		diags = diags.Append(valDiags.InConfigBody(n.Config.Config, n.Addr.String()))
-    		deprecationDiags := ctx.Deprecations().ValidateAsConfig(configVal, n.Schema.ConfigSchema, n.ModulePath())
+    		var deprecationDiags tfdiags.Diagnostics
+    		configVal, deprecationDiags = ctx.Deprecations().ValidateConfig(configVal, n.Schema.ConfigSchema, n.ModulePath())
     		diags = diags.Append(deprecationDiags.InConfigBody(n.Config.Config, n.Addr.String()))
     		if diags.HasErrors() {
@@ Expand Down @@

internal/terraform/node_action_partialexp.go

            
                      Original file line number
                      Diff line number
                      Diff line change
                  
    @@ -68,9 +68,9 @@ func (n *NodeActionDeclarationPartialExpanded) Execute(ctx EvalContext, op walkO
  
    		if diags.HasErrors() {

    			return diags

    		}

    		deprecationDiags := ctx.Deprecations().ValidateAsConfig(configVal, n.Schema.ConfigSchema, n.ActionAddr().Module)

    		diags = diags.Append(deprecationDiags)

    		var deprecationDiags tfdiags.Diagnostics

    		configVal, deprecationDiags = ctx.Deprecations().ValidateConfig(configVal, n.Schema.ConfigSchema, n.ActionAddr().Module)

    		diags = diags.Append(deprecationDiags.InConfigBody(n.config.Config, n.ActionAddr().String()))

    		if diags.HasErrors() {

    			return diags

    		}

internal/terraform/node_action_validate.go

-Original file line number
+Diff line change
@@ Expand Up @@
     			return diags
     		}
     	}
-    	diags = diags.Append(ctx.Deprecations().ValidateAsConfig(configVal, schema.ConfigSchema, n.ModulePath()).InConfigBody(n.Config.Config, n.Addr.String()))
+    	var deprecationDiags tfdiags.Diagnostics
+    	configVal, deprecationDiags = ctx.Deprecations().ValidateConfig(configVal, schema.ConfigSchema, n.ModulePath())
+    	diags = diags.Append(deprecationDiags.InConfigBody(n.Config.Config, n.Addr.String()))
     	valDiags = validateResourceForbiddenEphemeralValues(ctx, configVal, schema.ConfigSchema)
     	diags = diags.Append(valDiags.InConfigBody(config, n.Addr.String()))
@@ Expand Down @@

internal/terraform/node_local.go

-Original file line number
+Diff line change
@@ Expand Up / @@ -140,6 +140,10 @@ func (n *NodeLocal) References() []*addrs.Reference { @@
     func (n *NodeLocal) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) {
     	namedVals := ctx.NamedValues()
     	val, diags := evaluateLocalValue(n.Config, n.Addr.LocalValue, n.Addr.String(), ctx)
+    	// We only use a shallow evaluation of deprecations here because we only want to warn
+    	// if the entire value is deprecated. If e.g. a module is stored in the local and the module
+    	// contains a deprecated output we don't want to warn about that here, but only when the
+    	// output is actually referenced.
     	valWithoutDeprecations, deprecationDiags := ctx.Deprecations().Validate(val, n.ModulePath(), n.Config.Expr.Range().Ptr())
     	diags = diags.Append(deprecationDiags)
@@ Expand Down @@

internal/terraform/node_output.go

            
                      Original file line number
                      Diff line number
                      Diff line change
                  
    @@ -530,7 +530,7 @@ If you do intend to export this data, annotate the output value as sensitive by
  
    		}

    	} else if n.Config.Expr != nil {

    		var deprecationDiags tfdiags.Diagnostics

    		val, deprecationDiags = ctx.Deprecations().ValidateDeep(val, n.ModulePath(), n.Config.Expr.Range().Ptr())

    		val, deprecationDiags = ctx.Deprecations().ValidateExpressionDeep(val, n.ModulePath(), n.Config.Expr)

    		diags = diags.Append(deprecationDiags)

    	}

internal/terraform/node_provider.go

-Original file line number
+Diff line change
@@ Expand Up @@
     		return diags
     	}
-    	deprecationDiags := ctx.Deprecations().ValidateAsConfig(configVal, configSchema, n.Addr.Module)
+    	var deprecationDiags tfdiags.Diagnostics
+    	configVal, deprecationDiags = ctx.Deprecations().ValidateConfig(configVal, configSchema, n.Addr.Module)
     	diags = diags.Append(deprecationDiags.InConfigBody(configBody, n.Addr.String()))
     	if diags.HasErrors() {
     		return diags
@@ Expand Down @@

internal/terraform/node_resource_abstract_instance.go

-Original file line number
+Diff line change
@@ Expand Up / @@ -866,7 +866,9 @@ func (n *NodeAbstractResourceInstance) plan( @@
     	diags = diags.Append(
     		validateResourceForbiddenEphemeralValues(ctx, origConfigVal, schema.Body).InConfigBody(n.Config.Config, n.Addr.String()),
     	)
-    	diags = diags.Append(ctx.Deprecations().ValidateAsConfig(origConfigVal, schema.Body, n.ModulePath()).InConfigBody(n.Config.Config, n.Addr.String()))
+    	var deprecationDiags tfdiags.Diagnostics
+    	origConfigVal, deprecationDiags = ctx.Deprecations().ValidateConfig(origConfigVal, schema.Body, n.ModulePath())
+    	diags = diags.Append(deprecationDiags.InConfigBody(n.Config.Config, n.Addr.String()))
     	if diags.HasErrors() {
     		return nil, nil, deferred, keyData, diags
     	}
@@ Expand Down Expand Up @@
     				var configDiags tfdiags.Diagnostics
     				metaConfigVal, _, configDiags = ctx.EvaluateBlock(m.Config, providerSchema.ProviderMeta.Body, nil, EvalDataForNoInstanceKey)
     				diags = diags.Append(configDiags)
-    				diags = diags.Append(
-    					ctx.Deprecations().ValidateAsConfig(metaConfigVal, providerSchema.ProviderMeta.Body, ctx.Path().Module()).InConfigBody(m.Config, n.Addr.String()),
-    				)
-    				metaConfigVal = marks.RemoveDeprecationMarks(metaConfigVal)
+    				var deprecationDiags tfdiags.Diagnostics
+    				metaConfigVal, deprecationDiags = ctx.Deprecations().ValidateConfig(metaConfigVal, providerSchema.ProviderMeta.Body, ctx.Path().Module())
+    				diags = diags.Append(deprecationDiags.InConfigBody(m.Config, n.Addr.String()))
     			}
     		}
     	}
@@ Expand Down Expand Up @@
     	diags = diags.Append(
     		validateResourceForbiddenEphemeralValues(ctx, configVal, schema.Body).InConfigBody(n.Config.Config, n.Addr.String()),
     	)
-    	diags = diags.Append(
-    		ctx.Deprecations().ValidateAsConfig(configVal, schema.Body, ctx.Path().Module()).InConfigBody(n.Config.Config, n.Addr.String()),
-    	)
-    	configVal = marks.RemoveDeprecationMarks(configVal)
+    	var deprecationDiags tfdiags.Diagnostics
+    	configVal, deprecationDiags = ctx.Deprecations().ValidateConfig(configVal, schema.Body, ctx.Path().Module())
+    	diags = diags.Append(deprecationDiags.InConfigBody(n.Config.Config, n.Addr.String()))
     	if diags.HasErrors() {
     		return nil, nil, deferred, keyData, diags
     	}
@@ Expand Down Expand Up @@
     		return nil, keyData, diags
     	}
-    	diags = diags.Append(
-    		ctx.Deprecations().ValidateAsConfig(configVal, schema.Body, n.ModulePath()).InConfigBody(n.Config.Config, n.Addr.String()),
-    	)
+    	var deprecationDiags tfdiags.Diagnostics
+    	configVal, deprecationDiags = ctx.Deprecations().ValidateConfig(configVal, schema.Body, n.ModulePath())
+    	diags = diags.Append(deprecationDiags.InConfigBody(n.Config.Config, n.Addr.String()))
     	if diags.HasErrors() {
     		return nil, keyData, diags
     	}
-    	configVal = marks.RemoveDeprecationMarks(configVal)
     	newVal, readDeferred, readDiags := n.readDataSource(ctx, configVal)
     	if check, nested := n.nestedInCheckBlock(); nested {
@@ Expand Down Expand Up @@
     	config, _, configDiags := ctx.EvaluateBlock(body, schema, n.ResourceInstanceAddr().Resource, keyData)
     	diags = diags.Append(configDiags)
-    	diags = diags.Append(ctx.Deprecations().ValidateAsConfig(config, schema, n.ModulePath()).InConfigBody(body, n.Addr.String()))
-    	config = marks.RemoveDeprecationMarks(config)
+    	var deprecationDiags tfdiags.Diagnostics
+    	config, deprecationDiags = ctx.Deprecations().ValidateConfig(config, schema, n.ModulePath())
+    	diags = diags.Append(deprecationDiags.InConfigBody(body, n.Addr.String()))
     	return config, diags
     }
@@ Expand All @@
     	evalScope := ctx.EvaluationScope(n.ResourceInstanceAddr().Resource, nil, keyData)
     	config, evalDiags := evalScope.EvalSelfBlock(body, self, schema, keyData)
     	diags = diags.Append(evalDiags)
-    	diags = diags.Append(ctx.Deprecations().ValidateAsConfig(config, schema, n.ModulePath()).InConfigBody(body, n.Addr.String()))
-    	config = marks.RemoveDeprecationMarks(config)
+    	var deprecationDiags tfdiags.Diagnostics
+    	config, deprecationDiags = ctx.Deprecations().ValidateConfig(config, schema, n.ModulePath())
+    	diags = diags.Append(deprecationDiags.InConfigBody(body, n.Addr.String()))
     	return config, diags
     }
@@ Expand Down @@

internal/terraform/node_resource_ephemeral.go

-Original file line number
+Diff line change
@@ Expand Up @@
     	if diags.HasErrors() {
     		return nil, diags
     	}
-    	diags = diags.Append(ctx.Deprecations().ValidateAsConfig(configVal, schema.Body, ctx.Path().Module()).InConfigBody(config.Config, inp.addr.String()))
+    	var deprecationDiags tfdiags.Diagnostics
+    	configVal, deprecationDiags = ctx.Deprecations().ValidateConfig(configVal, schema.Body, ctx.Path().Module())
+    	diags = diags.Append(deprecationDiags.InConfigBody(config.Config, inp.addr.String()))
     	if diags.HasErrors() {
     		return nil, diags
     	}
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deprecation: limit deep unmarking for better performance #38082

Diff view

Diff view

There are no files selected for viewing

Uh oh!

deprecation: limit deep unmarking for better performance #38082

Are you sure you want to change the base?

deprecation: limit deep unmarking for better performance #38082

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!