Skip to content

Avoid reflect.MethodByName #863

Open

Description

@vincentbernat

Hey!

When using reflect.Method() or reflect.MethodByName() with a non-constant argument, dead code elimination is disabled for public functions. This makes the final binary bigger than it should. See for example kubernetes/kubernetes#132216 for an effort on the subject.

I am using the expr.Function() to declare function, so I think it should be possible to avoid calling reflect.MethodByName(). Would you consider a tag "exprdeclarative" or something like that to opt out reflect?

This would also disable some builtins, like get.

Activity

  1. vincentbernat commented on Nov 21, 2025

    @vincentbernat
    ContributorAuthor

    FI, I have just remove code using the culprit methods from expr: master...vincentbernat:expr:feature/dce-elimination. My own tests still pass. This is to show what could be possible.

  2. antonmedv commented on Nov 21, 2025

    @antonmedv
    Member

    With expr.Function() no reflect.MethodByName() is needed. But reflect.MethodByName() is kept as a fallback for other calls. There is no way to tell linter what this code will not be reachable.

    One of the solutions is to introduce build tag which will hide reflect.MethodByName() usage.

  3. vincentbernat commented on Nov 21, 2025

    @vincentbernat
    ContributorAuthor

    Yes, that was my point. Would you be OK with such a tag?

  4. antonmedv commented on Nov 21, 2025

    @antonmedv
    Member

    Yes, we just need to find a good name (expr_no_method_by_name?) and documentation.

  5. vincentbernat commented on Nov 21, 2025

    @vincentbernat
    ContributorAuthor

    OK, I can do that. My idea is to have reflect_on.go and reflectmethod_off.go, one with const disableReflectMethod := true and the other with const disableReflectMethod := false, and use this const in conditionals and rely on Go dead code elimination. Tag could be expr_no_reflectmethod (both .Method() and .MethodByName() should be removed).

  6. vincentbernat commented on Nov 21, 2025

    @vincentbernat
    ContributorAuthor

    I think I may have been too optimistic about my ability to do that. For example, looking at:

    func (c *compiler) CallNode(node *ast.CallNode) {
    	fn := node.Callee.Type()
    	if fn.Kind() == reflect.Func {
    		fnInOffset := 0
    		fnNumIn := fn.NumIn()
    		switch callee := node.Callee.(type) {
    		case *ast.MemberNode:
    			if prop, ok := callee.Property.(*ast.StringNode); ok {
    				if _, ok = callee.Node.Type().MethodByName(prop.Value); ok && callee.Node.Type().Kind() != reflect.Interface {
    					fnInOffset = 1
    					fnNumIn--
    				}
    			}
    		case *ast.IdentifierNode:
    			if t, ok := c.config.Env.MethodByName(c.ntCache, callee.Value); ok && t.Method {
    				fnInOffset = 1
    				fnNumIn--
    			}
    		}
    		for i, arg := range node.Arguments {
    			c.compile(arg)
    
    			var in reflect.Type
    			if fn.IsVariadic() && i >= fnNumIn-1 {
    				in = fn.In(fn.NumIn() - 1).Elem()
    			} else {
    				in = fn.In(i + fnInOffset)
    			}
    
    			c.derefParam(in, arg)
    		}
    	} else {
    		for _, arg := range node.Arguments {
    			c.compile(arg)
    		}
    	}
    
    	if ident, ok := node.Callee.(*ast.IdentifierNode); ok {
    		if c.config != nil {
    			if fn, ok := c.config.Functions[ident.Value]; ok {
    				c.emitFunction(fn, len(node.Arguments))
    				return
    			}
    		}
    	}
    	c.compile(node.Callee)
    
    	if c.config != nil {
    		isMethod, _, _ := checker.MethodIndex(c.ntCache, c.config.Env, node.Callee)
    		if index, ok := checker.TypedFuncIndex(node.Callee.Type(), isMethod); ok {
    			c.emit(OpCallTyped, index)
    			return
    		} else if checker.IsFastFunc(node.Callee.Type(), isMethod) {
    			c.emit(OpCallFast, len(node.Arguments))
    		} else {
    			c.emit(OpCall, len(node.Arguments))
    		}
    	} else {
    		c.emit(OpCall, len(node.Arguments))
    	}
    }

    I know the bottom part of the function is using expr.Function() but for the upper part, I don't know exactly if the whole if/else should be removed, only the if part or only the switch/case. I think only the if part should be removed, but I can't be really sure. It seems that I will have to guess everything and this is unlikely to be correct.

    I have updated the branch master...vincentbernat:expr:feature/dce-elimination with my current situation. Next steps would be to adapt the tests plus add a test to avoid a regression.

  7. antonmedv commented on Nov 21, 2025

    @antonmedv
    Member

    I see. I will try to work on this park as well.

  8. ofek-frame commented on Mar 31, 2026

    @ofek-frame

    Hey - any news on this front? We're encountering the same issue of DCE being disabled because of it.
    Thank you!

  9. added 4 commits that reference this issue on Apr 25, 2026
    1caa8f8
    7c188ce
    d0afa56
    ebfc047
  10. added 2 commits that reference this issue on Aug 11, 2026
    fb4cf1c
    ae7af0b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions