Repository: opentofu/opentofu
File: internal/tofu/transform_destroy_edge.go (Go)

Code before:
				n := nodes[i]
				switch n := n.(type) {
				case graphNodeTemporaryValue:
					// root module outputs indicate they are not temporary by
					// returning false here.
					if !n.temporaryValue() {
						return
					}

					// temporary values, which consist of variables, locals,
					// and outputs, must be kept if anything refers to them.
					for _, v := range g.UpEdges(n) {
						// keep any value which is connected through a
						// reference
						if _, ok := v.(GraphNodeReferencer); ok {
							return
						}
					}

				case graphNodeExpandsInstances:
					// Any nodes that expand instances are kept when their
					// instances may need to be evaluated.
					for _, v := range g.UpEdges(n) {
						switch v.(type) {
						case graphNodeExpandsInstances, GraphNodeDynamicExpandable:
							// Root module output values (which the following
							// condition matches) are exempt because we know
							// there is only ever exactly one instance of the
							// root module, and so it's not actually important
							// to expand it and so this lets us do a bit more
							// pruning than we'd be able to do otherwise.
							if tmp, ok := v.(graphNodeTemporaryValue); ok && !tmp.temporaryValue() {
								continue
							}

							// expanders can always depend on module expansion
							// themselves
							return
						case GraphNodeResourceInstance:
							// resource instances always depend on their
							// resource node, which is an expander
							return
						}
					}

				case GraphNodeProvider:
					// Only keep providers for evaluation if they have
					// resources to handle.
					// The provider transformers removed most unused providers
					// earlier, however there may be more to prune now based on
					// targeting or a destroy with no related instances in the

Review comment (suggestion):
I must admit that I don't recall the specific reason for it, but "expands instances" was added a long time after "dynamic expandable" to solve a specific problem. If it had been sufficient to use the original interface I expect that would've been the original design here. So I think we ought to carefully justify this change to make sure we're not undoing whatever this extra interface was intended to achieve. 🤔

Almost every node type in the initial graph is "dynamic expandable" and so I expect this will match far more nodes than the old code did. I think we're gonna need to dig into the history here and try to understand what motivated the current design to feel confident about making this change, since the implications of this could be quite subtle. 😖

I'll see if I can find some clues in the git history that might allow me to say something more specific than just waving a scary flag.

Code after:
			func() {
				n := nodes[i]
				switch n := n.(type) {
				case graphNodeTemporaryValue:
					// root module outputs indicate they are not temporary by
					// returning false here.
					if !n.temporaryValue() {
						return
					}

					// temporary values, which consist of variables, locals,
					// and outputs, must be kept if anything refers to them.
					for _, v := range g.UpEdges(n) {
						// keep any value which is connected through a
						// reference
						if _, ok := v.(GraphNodeReferencer); ok {
							return
						}
					}

				case graphNodeExpandsInstances:
					// Any nodes that expand instances are kept when their
					// instances may need to be evaluated.
					for _, v := range g.UpEdges(n) {
						switch v.(type) {
						case graphNodeExpandsInstances, GraphNodeDynamicExpandable:
							// Root module output values or checks (which the following
							// condition matches) are exempt because we know
							// there is only ever exactly one instance of the
							// root module, and so it's not actually important
							// to expand it and so this lets us do a bit more
							// pruning than we'd be able to do otherwise.
							if tmp, ok := v.(graphNodeTemporaryValue); ok && !tmp.temporaryValue() {
								continue
							}

							// expanders can always depend on module expansion
							// themselves
							return
						case GraphNodeResourceInstance:
							// resource instances always depend on their
							// resource node, which is an expander
							return
						}
					}

				case GraphNodeProvider:
					// Only keep providers for evaluation if they have
					// resources to handle.
					// The provider transformers removed most unused providers
					// earlier, however there may be more to prune now based on